From 275f434392d6e04ece74ddec70abde75c6d86603 Mon Sep 17 00:00:00 2001 From: Vlad Lesin Date: Tue, 12 Sep 2023 22:17:13 +0300 Subject: [PATCH] MDEV-25163 Rowid filter does not process storage engine error correctly. The fix is to return 3-state value from Range_rowid_filter::build() call: 1. The filter was built successfully; 2. The filter was not built, but the error was not fatal, i.e. there is no need to rollback transaction. For example, if the size of container to storevrow ids is not enough; 3. The filter was not built because of fatal error, for example, deadlock or lock wait timeout from storage engine. In this case we should stop query plan execution and roll back transaction. Reviewed by: Sergey Petrunya --- mysql-test/main/rowid_filter_innodb.result | 29 ++++++++++++ mysql-test/main/rowid_filter_innodb.test | 35 +++++++++++++++ sql/rowid_filter.cc | 52 +++++++++++++++------- sql/rowid_filter.h | 11 +++-- sql/sql_join_cache.cc | 6 ++- sql/sql_select.cc | 26 ++++++++--- sql/sql_select.h | 2 +- 7 files changed, 132 insertions(+), 29 deletions(-) diff --git a/mysql-test/main/rowid_filter_innodb.result b/mysql-test/main/rowid_filter_innodb.result index 1bf63d9a378..6bc00da6f4c 100644 --- a/mysql-test/main/rowid_filter_innodb.result +++ b/mysql-test/main/rowid_filter_innodb.result @@ -2717,6 +2717,35 @@ SELECT a FROM t1 WHERE c < 'k' AND b > 't' ORDER BY a; a 1 5 +# +# MDEV-25163 Rowid filter does not process storage engine error correctly. +# +CREATE TABLE two(tw int) Engine=MyISAM; +INSERT INTO two VALUES (1),(2); +SET GLOBAL innodb_stats_persistent= OFF; +EXPLAIN EXTENDED +SELECT a FROM t1 WHERE c < 'k' AND b > 't' ORDER BY a FOR UPDATE; +id select_type table type possible_keys key key_len ref rows filtered Extra +1 SIMPLE t1 range|filter b,c b|c 13|1027 NULL 5 (42%) 41.67 Using index condition; Using where; Using filesort; Using rowid filter +Warnings: +Note 1003 select `test`.`t1`.`a` AS `a` from `test`.`t1` where `test`.`t1`.`c` < 'k' and `test`.`t1`.`b` > 't' order by `test`.`t1`.`a` for update +EXPLAIN EXTENDED +SELECT a FROM t1,two WHERE c < 'k' AND b > 't' ORDER BY a FOR UPDATE; +id select_type table type possible_keys key key_len ref rows filtered Extra +1 SIMPLE two ALL NULL NULL NULL NULL 2 100.00 Using temporary; Using filesort +1 SIMPLE t1 range|filter b,c b|c 13|1027 NULL 5 (42%) 41.67 Using index condition; Using where; Using join buffer (flat, BNL join); Using rowid filter +Warnings: +Note 1003 select `test`.`t1`.`a` AS `a` from `test`.`t1` join `test`.`two` where `test`.`t1`.`c` < 'k' and `test`.`t1`.`b` > 't' order by `test`.`t1`.`a` for update +SET @saved_dbug = @@SESSION.debug_dbug; +SET debug_dbug = '+d,innodb_report_deadlock'; +SELECT a FROM t1 WHERE c < 'k' AND b > 't' ORDER BY a FOR UPDATE; +ERROR 40001: Deadlock found when trying to get lock; try restarting transaction +SET debug_dbug = @saved_dbug; +SET debug_dbug = '+d,innodb_report_deadlock'; +SELECT a FROM t1,two WHERE c < 'k' AND b > 't' ORDER BY a FOR UPDATE; +ERROR 40001: Deadlock found when trying to get lock; try restarting transaction +SET debug_dbug = @saved_dbug; +DROP TABLE two; DROP TABLE t1; SET GLOBAL innodb_stats_persistent= @stats.save; # diff --git a/mysql-test/main/rowid_filter_innodb.test b/mysql-test/main/rowid_filter_innodb.test index f4d0b241d11..d5f3e4d4a6b 100644 --- a/mysql-test/main/rowid_filter_innodb.test +++ b/mysql-test/main/rowid_filter_innodb.test @@ -1,5 +1,6 @@ --source include/no_valgrind_without_big.inc --source include/have_innodb.inc +--source include/have_debug.inc SET SESSION STORAGE_ENGINE='InnoDB'; @@ -132,6 +133,40 @@ SELECT a FROM t1 WHERE c < 'k' AND b > 't' ORDER BY a; SELECT a FROM t1 WHERE c < 'k' AND b > 't' ORDER BY a; +--echo # +--echo # MDEV-25163 Rowid filter does not process storage engine error correctly. +--echo # + +CREATE TABLE two(tw int) Engine=MyISAM; +INSERT INTO two VALUES (1),(2); + +# Switch off statistics update to catch correct create lock function call +SET GLOBAL innodb_stats_persistent= OFF; + +# Make sure it's still rowid filter + filesort +EXPLAIN EXTENDED +SELECT a FROM t1 WHERE c < 'k' AND b > 't' ORDER BY a FOR UPDATE; + +EXPLAIN EXTENDED +SELECT a FROM t1,two WHERE c < 'k' AND b > 't' ORDER BY a FOR UPDATE; + +SET @saved_dbug = @@SESSION.debug_dbug; +# Deadlock error should be returned from InnoDB during rowid filter container +# filling. If MDEV-25163 is not fixed, there will be assertion failure in +# InnoDB, as allready rolled back trx_t object will be used in filesort +# operation. +SET debug_dbug = '+d,innodb_report_deadlock'; +--error ER_LOCK_DEADLOCK +SELECT a FROM t1 WHERE c < 'k' AND b > 't' ORDER BY a FOR UPDATE; +SET debug_dbug = @saved_dbug; + +# Same as above for the join query: +SET debug_dbug = '+d,innodb_report_deadlock'; +--error ER_LOCK_DEADLOCK +SELECT a FROM t1,two WHERE c < 'k' AND b > 't' ORDER BY a FOR UPDATE; +SET debug_dbug = @saved_dbug; + +DROP TABLE two; DROP TABLE t1; SET GLOBAL innodb_stats_persistent= @stats.save; diff --git a/sql/rowid_filter.cc b/sql/rowid_filter.cc index 75f2ab71ecd..9ca2d3849f3 100644 --- a/sql/rowid_filter.cc +++ b/sql/rowid_filter.cc @@ -536,8 +536,11 @@ TABLE::best_range_rowid_filter_for_partial_join(uint access_key_no, range filter and place into the filter the rowids / primary keys read from key tuples when doing this scan. @retval - false on success - true otherwise + Rowid_filter::SUCCESS on success + Rowid_filter::NON_FATAL_ERROR the error which does not require transaction + rollback + Rowid_filter::FATAL_ERROR the error which does require transaction + rollback @note The function assumes that the quick select object to perform @@ -550,9 +553,9 @@ TABLE::best_range_rowid_filter_for_partial_join(uint access_key_no, purposes to facilitate a lazy building of the filter. */ -bool Range_rowid_filter::fill() +Rowid_filter::build_return_code Range_rowid_filter::build() { - int rc= 0; + build_return_code rc= SUCCESS; handler *file= table->file; THD *thd= table->in_use; QUICK_RANGE_SELECT* quick= (QUICK_RANGE_SELECT*) select->quick; @@ -573,18 +576,34 @@ bool Range_rowid_filter::fill() table->file->ha_start_keyread(quick->index); if (quick->init() || quick->reset()) - rc= 1; - - while (!rc) + rc= FATAL_ERROR; + else { - rc= quick->get_next(); - if (thd->killed) - rc= 1; - if (!rc) + for (;;) { + int quick_get_next_result= quick->get_next(); + if (thd->killed) + { + rc= FATAL_ERROR; + break; + } + if (quick_get_next_result != 0) + { + rc= (quick_get_next_result == HA_ERR_END_OF_FILE ? SUCCESS + : FATAL_ERROR); + /* + The error state has been set by file->print_error(res, MYF(0)) call + inside quick->get_next() call, in Mrr_simple_index_reader::get_next() + */ + DBUG_ASSERT(rc == SUCCESS || thd->is_error()); + break; + } file->position(quick->record); - if (container->add(NULL, (char*) file->ref)) - rc= 1; + if (container->add(NULL, (char *) file->ref)) + { + rc= NON_FATAL_ERROR; + break; + } else tracker->increment_container_elements_count(); } @@ -599,10 +618,9 @@ bool Range_rowid_filter::fill() file->in_range_check_pushed_down= in_range_check_pushed_down_save; tracker->report_container_buff_size(table->file->ref_length); - if (rc != HA_ERR_END_OF_FILE) - return 1; - table->file->rowid_filter_is_active= true; - return 0; + if (rc == SUCCESS) + table->file->rowid_filter_is_active= true; + return rc; } diff --git a/sql/rowid_filter.h b/sql/rowid_filter.h index eabf1f05fec..1e9b2eb5971 100644 --- a/sql/rowid_filter.h +++ b/sql/rowid_filter.h @@ -217,6 +217,11 @@ protected: Rowid_filter_tracker *tracker; public: + enum build_return_code { + SUCCESS, + NON_FATAL_ERROR, + FATAL_ERROR, + }; Rowid_filter(Rowid_filter_container *container_arg) : container(container_arg) {} @@ -224,7 +229,7 @@ public: Build the filter : fill it with info on the set of elements placed there */ - virtual bool build() = 0; + virtual build_return_code build() = 0; /* Check whether an element is in the filter. @@ -269,7 +274,7 @@ public: ~Range_rowid_filter(); - bool build() { return fill(); } + build_return_code build(); bool check(char *elem) { @@ -280,8 +285,6 @@ public: return was_checked; } - bool fill(); - SQL_SELECT *get_select() { return select; } }; diff --git a/sql/sql_join_cache.cc b/sql/sql_join_cache.cc index f1dd23d9618..e7b096b420b 100644 --- a/sql/sql_join_cache.cc +++ b/sql/sql_join_cache.cc @@ -2338,7 +2338,11 @@ enum_nested_loop_state JOIN_CACHE::join_matching_records(bool skip_last) if ((rc= join_tab_execution_startup(join_tab)) < 0) goto finish2; - join_tab->build_range_rowid_filter_if_needed(); + if (join_tab->build_range_rowid_filter_if_needed()) + { + rc= NESTED_LOOP_ERROR; + goto finish2; + } /* Prepare to retrieve all records of the joined table */ if (unlikely((error= join_tab_scan->open()))) diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 805ba19f0bc..d7badbc59a5 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -13626,8 +13626,18 @@ bool error_if_full_join(JOIN *join) } -void JOIN_TAB::build_range_rowid_filter_if_needed() +/** + Build rowid filter. + + @retval + 0 ok + @retval + 1 Error, transaction should be rolled back +*/ + +bool JOIN_TAB::build_range_rowid_filter_if_needed() { + bool result= false; if (rowid_filter && !is_rowid_filter_built) { /** @@ -13642,10 +13652,9 @@ void JOIN_TAB::build_range_rowid_filter_if_needed() Rowid_filter_tracker *rowid_tracker= rowid_filter->get_tracker(); table->file->set_time_tracker(rowid_tracker->get_time_tracker()); rowid_tracker->start_tracking(); - if (!rowid_filter->build()) - { + Rowid_filter::build_return_code build_rc= rowid_filter->build(); + if (build_rc == Rowid_filter::SUCCESS) is_rowid_filter_built= true; - } else { delete rowid_filter; @@ -13653,7 +13662,9 @@ void JOIN_TAB::build_range_rowid_filter_if_needed() } rowid_tracker->stop_tracking(); table->file->set_time_tracker(table_tracker); + result= (build_rc == Rowid_filter::FATAL_ERROR); } + return result; } @@ -20853,7 +20864,9 @@ sub_select(JOIN *join,JOIN_TAB *join_tab,bool end_of_records) if (!join_tab->preread_init_done && join_tab->preread_init()) DBUG_RETURN(NESTED_LOOP_ERROR); - join_tab->build_range_rowid_filter_if_needed(); + if (join_tab->build_range_rowid_filter_if_needed()) + DBUG_RETURN(NESTED_LOOP_ERROR); + if (join_tab->rowid_filter && join_tab->rowid_filter->is_empty()) rc= NESTED_LOOP_NO_MORE_ROWS; @@ -21810,7 +21823,8 @@ int join_init_read_record(JOIN_TAB *tab) if (tab->distinct && tab->remove_duplicates()) // Remove duplicates. return 1; - tab->build_range_rowid_filter_if_needed(); + if (tab->build_range_rowid_filter_if_needed()) + return 1; if (tab->filesort && tab->sort_table()) // Sort table. return 1; diff --git a/sql/sql_select.h b/sql/sql_select.h index 5c00a77638a..b0fa0b66b62 100644 --- a/sql/sql_select.h +++ b/sql/sql_select.h @@ -541,7 +541,7 @@ typedef struct st_join_table { /* Becomes true just after the used range filter has been built / filled */ bool is_rowid_filter_built; - void build_range_rowid_filter_if_needed(); + bool build_range_rowid_filter_if_needed(); void cleanup(); inline bool is_using_loose_index_scan()