[Commits] 61ad3ef: MDEV-19820 Wrong result with multiple single column index request
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. --- mysql-test/main/rowid_filter_innodb.result | 47 ++++++++++++++++++++++++++++++ mysql-test/main/rowid_filter_innodb.test | 30 +++++++++++++++++++ sql/sql_select.cc | 2 +- storage/innobase/row/row0sel.cc | 21 ++++++++++--- 4 files changed, 95 insertions(+), 5 deletions(-) diff --git a/mysql-test/main/rowid_filter_innodb.result b/mysql-test/main/rowid_filter_innodb.result index 54c7e03..d83239e 100644 --- a/mysql-test/main/rowid_filter_innodb.result +++ b/mysql-test/main/rowid_filter_innodb.result @@ -2162,4 +2162,51 @@ id select_type table type possible_keys key key_len ref rows filtered Extra Warnings: Note 1003 select 1 AS `id`,`test`.`t2`.`y` AS `y`,`test`.`t2`.`x` AS `x` from `test`.`t1` join `test`.`t2` where `test`.`t2`.`y` = 2 and `test`.`t2`.`x` = 1 drop table t1, t2; +# +# MDEV-19820: use of rowid filter for innodb table without primary key +# +create table t1 (a int, b int, key (b), key (a)) engine=innodb; +insert into t1 +select (rand(1)*1000)/10, (rand(1001)*1000)/50 from seq_1_to_1000; +analyze table t1; +Table Op Msg_type Msg_text +test.t1 analyze status Engine-independent statistics collected +test.t1 analyze status OK +set @save_optimizer_switch= @@optimizer_switch; +set optimizer_switch='rowid_filter=off'; +select count(*) from t1 where a in (22,83,11) and b=2; +count(*) +6 +explain extended select count(*) from t1 where a in (22,83,11) and b=2; +id select_type table type possible_keys key key_len ref rows filtered Extra +1 SIMPLE t1 ref b,a b 5 const 59 55.93 Using where +Warnings: +Note 1003 select count(0) AS `count(*)` from `test`.`t1` where `test`.`t1`.`b` = 2 and `test`.`t1`.`a` in (22,83,11) +select * from t1 where a in (22,83,11) and b=2; +a b +11 2 +11 2 +83 2 +11 2 +83 2 +22 2 +set optimizer_switch='rowid_filter=on'; +select count(*) from t1 where a in (22,83,11) and b=2; +count(*) +6 +explain extended select count(*) from t1 where a in (22,83,11) and b=2; +id select_type table type possible_keys key key_len ref rows filtered Extra +1 SIMPLE t1 ref|filter b,a b|a 5|5 const 59 (3%) 55.93 Using where; Using rowid filter +Warnings: +Note 1003 select count(0) AS `count(*)` from `test`.`t1` where `test`.`t1`.`b` = 2 and `test`.`t1`.`a` in (22,83,11) +select * from t1 where a in (22,83,11) and b=2; +a b +11 2 +11 2 +83 2 +11 2 +83 2 +22 2 +drop table t1; +set optimizer_switch=@save_optimizer_switch; SET SESSION STORAGE_ENGINE=DEFAULT; diff --git a/mysql-test/main/rowid_filter_innodb.test b/mysql-test/main/rowid_filter_innodb.test index 173ba15..240cd92 100644 --- a/mysql-test/main/rowid_filter_innodb.test +++ b/mysql-test/main/rowid_filter_innodb.test @@ -3,6 +3,7 @@ SET SESSION STORAGE_ENGINE='InnoDB'; --source rowid_filter.test +--source include/have_sequence.inc --echo # --echo # MDEV-18755: possible RORI-plan and possible plan with range filter @@ -65,4 +66,33 @@ eval explain extended $q; drop table t1, t2; +--echo # +--echo # MDEV-19820: use of rowid filter for innodb table without primary key +--echo # + +create table t1 (a int, b int, key (b), key (a)) engine=innodb; +insert into t1 +select (rand(1)*1000)/10, (rand(1001)*1000)/50 from seq_1_to_1000; +analyze table t1; + +let $q= +select count(*) from t1 where a in (22,83,11) and b=2; +let $q1= +select * from t1 where a in (22,83,11) and b=2; + +set @save_optimizer_switch= @@optimizer_switch; + +set optimizer_switch='rowid_filter=off'; +eval $q; +eval explain extended $q; +eval $q1; + +set optimizer_switch='rowid_filter=on'; +eval $q; +eval explain extended $q; +eval $q1; + +drop table t1; +set optimizer_switch=@save_optimizer_switch; + SET SESSION STORAGE_ENGINE=DEFAULT; diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 826026a..2382789 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -5382,7 +5382,7 @@ make_join_statistics(JOIN *join, List<TABLE_LIST> &tables_list, impossible_range= records == 0 && s->table->reginfo.impossible_range; if (join->thd->lex->sql_command == SQLCOM_SELECT && optimizer_flag(join->thd, OPTIMIZER_SWITCH_USE_ROWID_FILTER)) - s->table->init_cost_info_for_usable_range_rowid_filters(join->thd); + s->table->init_cost_info_for_usable_range_rowid_filters(join->thd); } if (!impossible_range) { 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
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
participants (2)
-
IgorBabaev
-
Marko Mäkelä