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.

Author: montywi

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 @@ class handler :public Sql_alloc
  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 @@ test_if_skip_sort_order(JOIN_TAB *tab,ORDER *order,ha_rows select_limit,
  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))
  {