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

@@ -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;
 #

mysql-test/main/rowid_filter_innodb.test

 --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;
 

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;
 }
 
 

sql/rowid_filter.h

@@ -217,6 +217,11 @@ class Rowid_filter : public Sql_alloc
   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 @@ class Rowid_filter : public Sql_alloc
     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 @@ class Range_rowid_filter: public Rowid_filter
 
   ~Range_rowid_filter();
 
-  bool build() { return fill(); }
+  build_return_code build();
 
   bool check(char *elem)
   {
@@ -280,8 +285,6 @@ class Range_rowid_filter: public Rowid_filter
     return was_checked;
   }
 
-  bool fill();
-
   SQL_SELECT *get_select() { return select; }
 };
 

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())))

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;

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()