MDEV-30944 Range_rowid_filter::fill() leaves file->keyread at MAX_KEY

This test case exposed 2 different bugs:
- When replacing a range with an index scan on a covering key
  in test_if_skip_sort_order() we didn't disable filtering.
  Filtering does not make much sense in this case.
  - Fixed by disabling filtering in this case.
- Range_rowid_filter::fill() did not take into account that keyread
  could already active, which caused an assert when it tried to
  activate another keyread.
  - Fixed by remembering old keyread state at start and restoring it
    at end.

Other things:
- ha_start_keyread() allowed multiple calls. This is wrong, especially
  as we do no check if the index changed!
  I added an assert() to ensure that we don't call it there is already
  an active keyread.
- ha_end_keyread() always called ha_extra(), even if keyread was not
  active.  Added a check to avoid the extra call.

mysql-test/main/rowid_filter_myisam.result

@@ -681,3 +681,17 @@ drop table t0;
 set optimizer_switch='rowid_filter=default';
 drop table name, flag2;
 drop table t1;
+#
+# MDEV-30944 Range_rowid_filter::fill() leaves file->keyread at MAX_KEY
+#
+CREATE TABLE t1 ( a varchar(30) , i int , id int, UNIQUE KEY id (id), KEY (i ,id ,a), KEY (a(1),i)) engine=myisam;
+INSERT INTO t1 VALUES('fej',NULL,1),('jeyw',8,2),(NULL,181,3),('wrkovd',9,4),('',NULL,5),('ko',NULL,6),('vdgzyxkop',217,7),('',7,8),('zy',0,9),('yxkopv',8,10),('kopv',1,11),('opv',4,12),('vc',9,13),('ri',1,14),('tkcn',1,15),('cnm',6,16),('m',0,17),('d',9,18),('e',28,19),(NULL,0,20);
+explain SELECT i, MAX( id ) FROM t1 WHERE ( a IS NULL OR a IN ('o','h') ) AND ( id BETWEEN 6 AND 7  OR id IN ( 8, 1) ) GROUP BY i;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	SIMPLE	t1	index	id,a	i	43	NULL	20	Using where; Using index
+SELECT i, MAX( id ) FROM t1 WHERE ( a IS NULL OR a IN ('o','h') ) AND ( id BETWEEN 6 AND 7  OR id IN ( 8, 1) ) GROUP BY i;
+i	MAX( id )
+drop table t1;
+#
+# End of 11.0 tests
+#

mysql-test/main/rowid_filter_myisam.test

@@ -2028,3 +2028,17 @@ set optimizer_switch='rowid_filter=default';
 
 drop table name, flag2;
 drop table t1;
+
+--echo #
+--echo # MDEV-30944 Range_rowid_filter::fill() leaves file->keyread at MAX_KEY
+--echo #
+
+CREATE TABLE t1 ( a varchar(30) , i int , id int, UNIQUE KEY id (id), KEY (i ,id ,a), KEY (a(1),i)) engine=myisam;
+INSERT INTO t1 VALUES('fej',NULL,1),('jeyw',8,2),(NULL,181,3),('wrkovd',9,4),('',NULL,5),('ko',NULL,6),('vdgzyxkop',217,7),('',7,8),('zy',0,9),('yxkopv',8,10),('kopv',1,11),('opv',4,12),('vc',9,13),('ri',1,14),('tkcn',1,15),('cnm',6,16),('m',0,17),('d',9,18),('e',28,19),(NULL,0,20);
+explain SELECT i, MAX( id ) FROM t1 WHERE ( a IS NULL OR a IN ('o','h') ) AND ( id BETWEEN 6 AND 7  OR id IN ( 8, 1) ) GROUP BY i;
+SELECT i, MAX( id ) FROM t1 WHERE ( a IS NULL OR a IN ('o','h') ) AND ( id BETWEEN 6 AND 7  OR id IN ( 8, 1) ) GROUP BY i;
+drop table t1;
+
+--echo #
+--echo # End of 11.0 tests
+--echo #

sql/handler.h

@@ -3486,19 +3486,38 @@ public:
   inline bool keyread_enabled() { return keyread < MAX_KEY; }
   inline int ha_start_keyread(uint idx)
   {
-    if (keyread_enabled())
-      return 0;
+    DBUG_ASSERT(!keyread_enabled());
     keyread= idx;
     return extra_opt(HA_EXTRA_KEYREAD, idx);
   }
   inline int ha_end_keyread()
   {
-    if (!keyread_enabled())
+    if (!keyread_enabled())                    /* Enably lazy usage */
       return 0;
     keyread= MAX_KEY;
     return extra(HA_EXTRA_NO_KEYREAD);
   }
 
+  /*
+    End any active keyread. Return state so that we can restore things
+    at end.
+  */
+  int ha_end_active_keyread()
+  {
+    int org_keyread;
+    if (!keyread_enabled())
+      return MAX_KEY;
+    org_keyread= keyread;
+    ha_end_keyread();
+    return org_keyread;
+  }
+  /* Restore state to before ha_end_active_keyread */
+  void ha_restart_keyread(int org_keyread)
+  {
+    DBUG_ASSERT(!keyread_enabled());
+    if (org_keyread != MAX_KEY)
+      ha_start_keyread(org_keyread);
+  }
   int check_collation_compatibility();
   int check_long_hash_compatibility() const;
   int ha_check_for_upgrade(HA_CHECK_OPT *check_opt);

sql/opt_range.cc

@@ -15801,7 +15801,8 @@ int QUICK_GROUP_MIN_MAX_SELECT::reset(void)
   DBUG_ENTER("QUICK_GROUP_MIN_MAX_SELECT::reset");
 
   seen_first_key= FALSE;
-  head->file->ha_start_keyread(index); /* We need only the key attributes */
+  if (!head->file->keyread_enabled())
+    head->file->ha_start_keyread(index); /* We need only the key attributes */
 
   if ((result= file->ha_index_init(index,1)))
   {

sql/rowid_filter.cc

@@ -580,11 +580,11 @@ bool Range_rowid_filter::fill()
   handler *file= table->file;
   THD *thd= table->in_use;
   QUICK_RANGE_SELECT* quick= (QUICK_RANGE_SELECT*) select->quick;
-
   uint table_status_save= table->status;
   Item *pushed_idx_cond_save= file->pushed_idx_cond;
   uint pushed_idx_cond_keyno_save= file->pushed_idx_cond_keyno;
   bool in_range_check_pushed_down_save= file->in_range_check_pushed_down;
+  int org_keyread;
 
   table->status= 0;
   file->pushed_idx_cond= 0;
@@ -594,6 +594,7 @@ bool Range_rowid_filter::fill()
   /* We're going to just read rowids / clustered primary keys */
   table->prepare_for_position();
 
+  org_keyread= file->ha_end_active_keyread();
   file->ha_start_keyread(quick->index);
 
   if (quick->init() || quick->reset())
@@ -612,6 +613,7 @@ bool Range_rowid_filter::fill()
 end:
   quick->range_end();
   file->ha_end_keyread();
+  file->ha_restart_keyread(org_keyread);
 
   table->status= table_status_save;
   file->pushed_idx_cond= pushed_idx_cond_save;

sql/sql_select.cc

@@ -26423,7 +26423,8 @@ check_reverse_order:
           if the table is accessed by the primary key
         */
         if (tab->rowid_filter &&
-            table->file->is_clustering_key(tab->index))
+            (table->file->is_clustering_key(tab->index) ||
+             table->covering_keys.is_set(best_key)))
           tab->clear_range_rowid_filter();
 
         if (tab->pre_idx_push_select_cond)

sql/table.cc

@@ -7477,7 +7477,7 @@ MY_BITMAP *TABLE::prepare_for_keyread(uint index, MY_BITMAP *map)
 {
   MY_BITMAP *backup= read_set;
   DBUG_ENTER("TABLE::prepare_for_keyread");
-  if (!no_keyread)
+  if (!no_keyread && !file->keyread_enabled())
     file->ha_start_keyread(index);
   if (map != read_set || !is_clustering_key(index))
   {