Hi Igor,
On Sun, Jun 23, 2019 at 8:26 PM IgorBabaev <i...@mariadb.com> 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
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
