Hi Igor, On Sun, Jun 23, 2019 at 8:26 PM IgorBabaev <igor@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