From e5c4c0842d0a11b9919efcda09377083a4a0d69a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Thu, 19 Dec 2024 14:05:16 +0200 Subject: [PATCH] MDEV-35443: opt_search_plan_for_table() may degrade to full table scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit opt_calc_index_goodness(): Correct an inaccurate condition. We can very well use a clustered index of a table that is subject to online rebuild. But we must not choose an index that has not been committed (it is a secondary index that was not fully created) or that is corrupted or not a normal B-tree index. opt_search_plan_for_table(): Remove some redundant code, now that opt_calc_index_goodness() checks against corrupted indexes. The test case allows this code to be exercised. The main observation in the following: ./mtr --rr innodb.stats_persistent rr replay var/log/mysqld.1.rr/latest-trace should be that when opt_search_plan_for_table() is being invoked by dict_stats_update_persistent() on the being-altered statistics table in the 2nd call after ha_innobase::inplace_alter_table(), and the fix in opt_calc_index_goodness() is absent, it would choose the code path if (n_fields == 0), that is, a full table scan, instead of searching for the record. The GDB commands to execute in "rr replay" would be as follows: break ha_innobase::inplace_alter_table continue break opt_search_plan_for_table continue continue next next … Reviewed by: Vladislav Lesin --- mysql-test/suite/innodb/r/stats_persistent.result | 9 +++++++++ mysql-test/suite/innodb/t/stats_persistent.test | 11 +++++++++++ storage/innobase/include/dict0mem.h | 8 ++++++++ storage/innobase/pars/pars0opt.cc | 13 +++++-------- 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/mysql-test/suite/innodb/r/stats_persistent.result b/mysql-test/suite/innodb/r/stats_persistent.result index 7e9c038d6f7..2752811a07c 100644 --- a/mysql-test/suite/innodb/r/stats_persistent.result +++ b/mysql-test/suite/innodb/r/stats_persistent.result @@ -6,14 +6,23 @@ SET DEBUG_SYNC='dict_stats_update_persistent SIGNAL stop WAIT_FOR go'; ANALYZE TABLE t1; connect con1, localhost, root; SET DEBUG_SYNC='now WAIT_FOR stop'; +connect con2, localhost, root; +SET DEBUG_SYNC='innodb_inplace_alter_table_enter SIGNAL astop WAIT_FOR ago'; +ALTER TABLE mysql.innodb_index_stats FORCE; +connection con1; SELECT SUM(DATA_LENGTH+INDEX_LENGTH) FROM information_schema.TABLES WHERE ENGINE='InnoDB'; SUM(DATA_LENGTH+INDEX_LENGTH) SUM +SET DEBUG_SYNC='now WAIT_FOR astop'; SET DEBUG_SYNC='now SIGNAL go'; disconnect con1; connection default; Table Op Msg_type Msg_text test.t1 analyze status Engine-independent statistics collected test.t1 analyze status OK +SET DEBUG_SYNC='now SIGNAL ago'; +connection con2; +disconnect con2; +connection default; SET DEBUG_SYNC= 'RESET'; DROP TABLE t1; diff --git a/mysql-test/suite/innodb/t/stats_persistent.test b/mysql-test/suite/innodb/t/stats_persistent.test index 8561298c4d3..870808993f4 100644 --- a/mysql-test/suite/innodb/t/stats_persistent.test +++ b/mysql-test/suite/innodb/t/stats_persistent.test @@ -14,14 +14,25 @@ SET DEBUG_SYNC='dict_stats_update_persistent SIGNAL stop WAIT_FOR go'; --connect(con1, localhost, root) SET DEBUG_SYNC='now WAIT_FOR stop'; +--connect(con2, localhost, root) +SET DEBUG_SYNC='innodb_inplace_alter_table_enter SIGNAL astop WAIT_FOR ago'; +send ALTER TABLE mysql.innodb_index_stats FORCE; + +--connection con1 --replace_column 1 SUM SELECT SUM(DATA_LENGTH+INDEX_LENGTH) FROM information_schema.TABLES WHERE ENGINE='InnoDB'; +SET DEBUG_SYNC='now WAIT_FOR astop'; SET DEBUG_SYNC='now SIGNAL go'; --disconnect con1 --connection default --reap +SET DEBUG_SYNC='now SIGNAL ago'; +--connection con2 +reap; +--disconnect con2 +--connection default SET DEBUG_SYNC= 'RESET'; DROP TABLE t1; diff --git a/storage/innobase/include/dict0mem.h b/storage/innobase/include/dict0mem.h index 5cebfe6710d..5ec2273ef65 100644 --- a/storage/innobase/include/dict0mem.h +++ b/storage/innobase/include/dict0mem.h @@ -1203,6 +1203,14 @@ public: /** @return whether this is the change buffer */ bool is_ibuf() const { return UNIV_UNLIKELY(type & DICT_IBUF); } + /** @return whether this is a normal, non-virtual B-tree index + (not the change buffer, not SPATIAL or FULLTEXT) */ + bool is_normal_btree() const noexcept { + return UNIV_LIKELY(!(type & (DICT_IBUF | DICT_SPATIAL + | DICT_FTS | DICT_CORRUPT + | DICT_VIRTUAL))); + } + /** @return whether the index includes virtual columns */ bool has_virtual() const { return type & DICT_VIRTUAL; } diff --git a/storage/innobase/pars/pars0opt.cc b/storage/innobase/pars/pars0opt.cc index e1a913b0179..a5ab15b74d4 100644 --- a/storage/innobase/pars/pars0opt.cc +++ b/storage/innobase/pars/pars0opt.cc @@ -340,9 +340,7 @@ opt_calc_index_goodness( ulint op; ulint j; - /* At least for now we don't support using FTS indexes for queries - done through InnoDB's own SQL parser. */ - if (dict_index_is_online_ddl(index) || (index->type & DICT_FTS)) { + if (!index->is_normal_btree() || !index->is_committed()) { return(0); } @@ -572,11 +570,10 @@ opt_search_plan_for_table( /* Calculate goodness for each index of the table */ index = dict_table_get_first_index(table); - best_index = index; /* Eliminate compiler warning */ + best_index = index; best_goodness = 0; - /* should be do ... until ? comment by Jani */ - while (index) { + do { goodness = opt_calc_index_goodness(index, sel_node, i, index_plan, &last_op); if (goodness > best_goodness) { @@ -590,8 +587,8 @@ opt_search_plan_for_table( best_last_op = last_op; } - dict_table_next_uncorrupted_index(index); - } + index = dict_table_get_next_index(index); + } while (index); plan->index = best_index;