maria-developers team mailing list archive

[Marko Mäkelä <marko.makela@xxxxxxxxxxx>]

Hi Igor,

On Sun, Jun 23, 2019 at 8:26 PM IgorBabaev <igor@xxxxxxxxxxx> wrote:
>
> revision-id: 61ad3efea00303e2d0cd85628c76251b08e79531 (mariadb-10.4.4-194-g61ad3ef)
> parent(s): 8b576616b442d061356bc5a2abd410f478e98ee7
> author: Igor Babaev
> committer: Igor Babaev
> timestamp: 2019-06-23 10:26:42 -0700
> message:
>
> MDEV-19820 Wrong result with multiple single column index request
>
> The bug occured when the optimizer decided to use a rowid filter built
> by a range index scan to access an InnoDB table with generated clustered
> index.
> When a table is accessed by a secondary index Idx employing a rowid filter the
> the value of pk contained in the found index tuple is checked against the
> filter. A call of the handler function position is supposed to put the
> pk value into the handler::ref buffer. However for generated clustered
> primary keys it did not happened. The patch fixes this problem.
[snip]
> diff --git a/storage/innobase/row/row0sel.cc b/storage/innobase/row/row0sel.cc
> index 659a824..d796b88 100644
> --- a/storage/innobase/row/row0sel.cc
> +++ b/storage/innobase/row/row0sel.cc
> @@ -3902,10 +3902,23 @@ row_search_idx_cond_check(
>
>         switch (result) {
>         case ICP_MATCH:
> -               if (handler_rowid_filter_is_active(prebuilt->pk_filter)
> -                   && !handler_rowid_filter_check(prebuilt->pk_filter)) {
> -                       MONITOR_INC(MONITOR_ICP_MATCH);
> -                       return(ICP_NO_MATCH);
> +               if (handler_rowid_filter_is_active(prebuilt->pk_filter)) {
> +                       if (prebuilt->clust_index_was_generated) {
> +                               ulint len;
> +                               dict_index_t* index = prebuilt->index;
> +                               const byte* data = rec_get_nth_field(
> +                                       rec, offsets, index->n_fields - 1,
> +                                       &len);
> +                               ut_ad(dict_index_get_nth_col(index,
> +                                                            index->n_fields - 1)
> +                                     ->prtype == (DATA_ROW_ID | DATA_NOT_NULL));
> +                               ut_ad(len == DATA_ROW_ID_LEN);
> +                               memcpy(prebuilt->row_id, data, DATA_ROW_ID_LEN);
> +                        }
> +                        if (!handler_rowid_filter_check(prebuilt->pk_filter)) {
> +                               MONITOR_INC(MONITOR_ICP_MATCH);
> +                               return(ICP_NO_MATCH);
> +                        }
>                 }
>                 /* Convert the remaining fields to MySQL format.
>                 If this is a secondary index record, we must defer

Here, the changed code is assuming that prebuilt->index is a secondary
index without explicitly saying so.
I do not see an assertion similar to

ut_ad(!handler_rowid_filter_is_active(prebuilt->pk_filter) ||
!prebuilt->index->is_primary());

anywhere in row0sel.cc or ha_innodb.cc. A natural place for such a
check could be in ha_innobase::build_template().

If the pk_filter is only being used for secondary index access, then I
think that it should be enforced by some debug assertion and with a
test case.

Another problem is that we are doing busy work if
handler_rowid_filter_check(prebuilt->pk_filter) does hold, and we will
not return ICP_NO_MATCH above.

I wonder if a simpler approach of reordering and deduplicating some
code would work:

diff --git a/storage/innobase/row/row0sel.cc b/storage/innobase/row/row0sel.cc
index 659a824a0d5..068dc849449 100644
--- a/storage/innobase/row/row0sel.cc
+++ b/storage/innobase/row/row0sel.cc
@@ -3902,11 +3902,6 @@ row_search_idx_cond_check(

     switch (result) {
     case ICP_MATCH:
-        if (handler_rowid_filter_is_active(prebuilt->pk_filter)
-            && !handler_rowid_filter_check(prebuilt->pk_filter)) {
-            MONITOR_INC(MONITOR_ICP_MATCH);
-            return(ICP_NO_MATCH);
-        }
         /* Convert the remaining fields to MySQL format.
         If this is a secondary index record, we must defer
         this until we have fetched the clustered index record. */
@@ -3920,6 +3915,10 @@ row_search_idx_cond_check(
             }
         }
         MONITOR_INC(MONITOR_ICP_MATCH);
+        if (handler_rowid_filter_is_active(prebuilt->pk_filter)
+            && !handler_rowid_filter_check(prebuilt->pk_filter)) {
+            result = ICP_NO_MATCH;
+        }
         return(result);
     case ICP_NO_MATCH:
         MONITOR_INC(MONITOR_ICP_NO_MATCH);

Can you try that?

With best regards,

Marko