MDEV-41351 Allow a covering index scan for a SELECT ... FOR UPDATE un…

mysql-test/main/innodb_full_scan,table_lock.rdiff

@@ -1,5 +1,5 @@
 --- innodb_full_scan.result
-+++ innodb_full_scan,table_lock.result
++++ innodb_full_scan,table_lock.reject
 @@ -10,14 +10,14 @@
  42
  COMMIT;
@@ -119,3 +119,19 @@
  drop table t10, t11;
  # Range access
  create table t10 (a int, b varchar(100), index(a)) engine=innodb;
+@@ -185,7 +185,7 @@
+ # 10000 rows; clustered path costs ~2/row
+ SELECT ($e - $s) < 10000 AS covering;
+ covering
+-0
++1
+ connect  con1,localhost,root,,;
+ SET innodb_lock_wait_timeout=1;
+ # Case 1: covering point lookup (never a full scan, no table lock)
+@@ -231,5 +231,5 @@
+ COMMIT;
+ SELECT ($e - $s) < 10000 AS covering;
+ covering
+-0
++1
+ DROP TABLE t2;

mysql-test/main/innodb_full_scan.result

@@ -174,3 +174,81 @@ insert into t11 select a from t10 where a > 30;
 # lock_rec_created
 11
 drop table t10, t11;
+#
+# MDEV-41351 Redundant clustered lookups in covering scans when holding a table X-lock
+#
+# Covering full scan
+CREATE TABLE t1 (id INT PRIMARY KEY, sec INT, pad CHAR(100), KEY i1(sec))
+ENGINE=InnoDB;
+INSERT INTO t1 SELECT seq, seq, seq FROM seq_1_to_10000;
+SELECT sec,id FROM t1 FOR UPDATE;
+# 10000 rows; clustered path costs ~2/row
+SELECT ($e - $s) < 10000 AS covering;
+covering
+0
+connect  con1,localhost,root,,;
+SET innodb_lock_wait_timeout=1;
+# Case 1: covering point lookup (never a full scan, no table lock)
+connection default;
+EXPLAIN SELECT sec,id FROM t1 WHERE sec=5 FOR UPDATE;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	SIMPLE	t1	ref	i1	i1	5	const	#	Using index
+BEGIN;
+SELECT sec,id FROM t1 WHERE sec=5 FOR UPDATE;
+sec	id
+5	5
+connection con1;
+# PK path, non-indexed column: must wait for A's clustered X lock
+UPDATE t1 SET pad='y' WHERE id=5;
+ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+connection default;
+COMMIT;
+# Case 2: covering full scan
+connection default;
+BEGIN;
+SELECT sec,id FROM t1 FOR UPDATE;
+connection con1;
+UPDATE t1 SET pad='y' WHERE id=5;
+ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+connection default;
+COMMIT;
+# locks released: the UPDATE now succeeds
+connection con1;
+UPDATE t1 SET pad='y' WHERE id=5;
+SELECT pad FROM t1 WHERE id=5;
+pad
+y
+disconnect con1;
+connection default;
+DROP TABLE t1;
+# Prefix case, row_search_with_covering_prefix coverage
+CREATE TABLE t2 (id INT PRIMARY KEY, a INT NOT NULL, v VARCHAR(50),
+KEY k(a, v(5))) ENGINE=InnoDB;
+INSERT INTO t2 SELECT seq, seq, 'x' FROM seq_1_to_10000;
+# LOCK_X, no read view
+BEGIN;
+SELECT id, a, v FROM t2 FORCE INDEX(k) ORDER BY a FOR UPDATE;
+COMMIT;
+SELECT ($e - $s) < 10000 AS covering;
+covering
+0
+DROP TABLE t2;
+#
+# A covering locking full scan must still visit the clustered index
+# when a snapshot isolation read view is open, so that a row changed
+# after the snapshot is reported as ER_CHECKREAD.
+#
+SET innodb_snapshot_isolation= ON;
+CREATE TABLE t1 (a INT PRIMARY KEY, b INT, KEY(b)) ENGINE=InnoDB STATS_PERSISTENT=0;
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+connect  con1,localhost,root,,;
+INSERT INTO t1 VALUES (1, 1);
+disconnect con1;
+connection default;
+EXPLAIN SELECT a, b FROM t1 FORCE INDEX (b) FOR UPDATE;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	SIMPLE	t1	index	NULL	b	5	NULL	#	Using index
+SELECT a, b FROM t1 FORCE INDEX (b) FOR UPDATE;
+ERROR HY000: Record has changed since last read in table 't1'; try restarting transaction
+COMMIT;
+DROP TABLE t1;

mysql-test/main/innodb_full_scan.test

@@ -289,3 +289,100 @@ let $result= `SELECT $end_val - $start_val`;
 echo # lock_rec_created;
 echo $result;
 drop table t10, t11;
+
+--echo #
+--echo # MDEV-41351 Redundant clustered lookups in covering scans when holding a table X-lock
+--echo #
+
+--echo # Covering full scan
+CREATE TABLE t1 (id INT PRIMARY KEY, sec INT, pad CHAR(100), KEY i1(sec))
+ENGINE=InnoDB;
+INSERT INTO t1 SELECT seq, seq, seq FROM seq_1_to_10000;
+
+let $bp= SELECT VARIABLE_VALUE FROM information_schema.GLOBAL_STATUS
+         WHERE VARIABLE_NAME='INNODB_BUFFER_POOL_READ_REQUESTS';
+let $s= `$bp`;
+--disable_result_log
+SELECT sec,id FROM t1 FOR UPDATE;
+--enable_result_log
+let $e= `$bp`;
+--echo # 10000 rows; clustered path costs ~2/row
+evalp SELECT ($e - $s) < 10000 AS covering;
+
+--connect (con1,localhost,root,,)
+SET innodb_lock_wait_timeout=1;
+
+--echo # Case 1: covering point lookup (never a full scan, no table lock)
+--connection default
+--replace_column 9 #
+EXPLAIN SELECT sec,id FROM t1 WHERE sec=5 FOR UPDATE;
+BEGIN;
+SELECT sec,id FROM t1 WHERE sec=5 FOR UPDATE;
+--connection con1
+--echo # PK path, non-indexed column: must wait for A's clustered X lock
+--error ER_LOCK_WAIT_TIMEOUT
+UPDATE t1 SET pad='y' WHERE id=5;
+--connection default
+COMMIT;
+
+--echo # Case 2: covering full scan
+--connection default
+BEGIN;
+--disable_result_log
+SELECT sec,id FROM t1 FOR UPDATE;
+--enable_result_log
+--connection con1
+--error ER_LOCK_WAIT_TIMEOUT
+UPDATE t1 SET pad='y' WHERE id=5;
+--connection default
+COMMIT;
+
+--echo # locks released: the UPDATE now succeeds
+--connection con1
+UPDATE t1 SET pad='y' WHERE id=5;
+SELECT pad FROM t1 WHERE id=5;
+--disconnect con1
+--connection default
+DROP TABLE t1;
+
+--echo # Prefix case, row_search_with_covering_prefix coverage
+CREATE TABLE t2 (id INT PRIMARY KEY, a INT NOT NULL, v VARCHAR(50),
+  KEY k(a, v(5))) ENGINE=InnoDB;
+INSERT INTO t2 SELECT seq, seq, 'x' FROM seq_1_to_10000;
+
+--echo # LOCK_X, no read view
+let $s= `$bp`;
+BEGIN;
+--disable_result_log
+SELECT id, a, v FROM t2 FORCE INDEX(k) ORDER BY a FOR UPDATE;
+--enable_result_log
+COMMIT;
+let $e= `$bp`;
+evalp SELECT ($e - $s) < 10000 AS covering;
+DROP TABLE t2;
+
+--echo #
+--echo # A covering locking full scan must still visit the clustered index
+--echo # when a snapshot isolation read view is open, so that a row changed
+--echo # after the snapshot is reported as ER_CHECKREAD.
+--echo #
+SET innodb_snapshot_isolation= ON;
+
+CREATE TABLE t1 (a INT PRIMARY KEY, b INT, KEY(b)) ENGINE=InnoDB STATS_PERSISTENT=0;
+
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+
+--connect (con1,localhost,root,,)
+INSERT INTO t1 VALUES (1, 1);
+--disconnect con1
+
+--connection default
+--replace_column 9 #
+EXPLAIN SELECT a, b FROM t1 FORCE INDEX (b) FOR UPDATE;
+--disable_ps2_protocol
+--error ER_CHECKREAD
+SELECT a, b FROM t1 FORCE INDEX (b) FOR UPDATE;
+--enable_ps2_protocol
+COMMIT;
+
+DROP TABLE t1;

storage/innobase/handler/ha_innodb.cc

@@ -6404,7 +6404,7 @@ ha_innobase::clone(
 		DBUG_ASSERT(new_handler->m_prebuilt != NULL);
 
 		new_handler->m_prebuilt->select_lock_type
-			= m_prebuilt->select_lock_type;
+			= m_prebuilt->real_lock_mode();
 	}
 
 	DBUG_RETURN(new_handler);
@@ -7668,7 +7668,8 @@ ha_innobase::build_template(
 		/* We always retrieve the whole clustered index record if we
 		use exclusive row level locks, for example, if the read is
 		done in an UPDATE statement or if we are using a no rollback
-                table */
+		table. LOCK_X_BY_TABLE (see	extra_opt()) is deliberately not
+		matched here */
 
 		whole_row = true;
 	} else if (!whole_row) {
@@ -16774,9 +16775,32 @@ int ha_innobase::extra_opt(enum ha_extra_function operation, ulong arg)
   switch (operation)
   {
     case HA_EXTRA_FULL_SCAN:
-      if (THDVAR(ha_thd(), table_lock_on_full_scan) &&
-          !m_prebuilt->skip_locked && arg == ULONG_MAX)
-        m_prebuilt->full_table_scan = true;
+      if (arg == ULONG_MAX && !m_prebuilt->skip_locked &&
+          THDVAR(m_user_thd, table_lock_on_full_scan))
+      {
+        m_prebuilt->full_table_scan= true;
+        /* Allow a covering index scan to be considered for a LOCK_X
+        read in a SELECT statement. Otherwise a LOCK_X read always
+        reads the whole clustered index record (see
+        ha_innobase::build_template()): UPDATE and DELETE need it to
+        write the row, and a locking read through a secondary index
+        must lock the clustered index record to lock the row against
+        access through any other index.
+
+        With innodb_snapshot_isolation and a read view already open
+        (e.g. inside a START TRANSACTION WITH CONSISTENT SNAPSHOT, or
+        an earlier consistent read in a REPEATABLE READ transaction),
+        it needs to check if there is a committed change the
+        transaction never saw. To that end it needs the record's
+        trx_id which is only available from clustered records, see
+        lock_clust_rec_read_check_and_lock. In such a case we need to
+        disable the optimization. */
+        if (m_prebuilt->select_lock_type == LOCK_X &&
+            thd_sql_command(ha_thd()) == SQLCOM_SELECT &&
+            !(m_prebuilt->trx->snapshot_isolation &&
+              m_prebuilt->trx->read_view.is_open()))
+          m_prebuilt->select_lock_type= LOCK_X_BY_TABLE;
+      }
       return 0;
     default:/* Do nothing */
       ;
@@ -16805,6 +16829,7 @@ ha_innobase::reset()
 
 	m_prebuilt->skip_locked = false;
 	m_prebuilt->full_table_scan = false;
+	m_prebuilt->select_lock_type = m_prebuilt->real_lock_mode();
 	return(0);
 }
 

storage/innobase/include/lock0priv.h

@@ -73,6 +73,7 @@ ib_lock_t::print(std::ostream& out) const
   static_assert(LOCK_IX == 1, "compatibility");
   static_assert(LOCK_S == 2, "compatibility");
   static_assert(LOCK_X == 3, "compatibility");
+  static_assert((LOCK_X_BY_TABLE & LOCK_MODE_MASK) == LOCK_X, "compatibility");
   static_assert(LOCK_AUTO_INC == 4, "compatibility");
   static_assert(LOCK_NONE == 5, "compatibility");
   static_assert(LOCK_NONE_UNSET == 7, "compatibility");

storage/innobase/include/lock0types.h

@@ -46,7 +46,12 @@ enum lock_mode {
 			in an exclusive mode */
 	LOCK_NONE,	/* this is used elsewhere to note consistent read */
 	LOCK_NUM = LOCK_NONE, /* number of lock modes */
-	LOCK_NONE_UNSET = 7
+	LOCK_NONE_UNSET = 7,
+	/* Only in row_prebuilt_t::select_lock_type: a LOCK_X read whose
+	record locks are made redundant by a table-level LOCK_X
+	(innodb_table_lock_on_full_scan), so a covering index may be used.
+	Masking with LOCK_MODE_MASK yields LOCK_X. */
+	LOCK_X_BY_TABLE = LOCK_X | 8
 };
 
 /** A table lock */

storage/innobase/include/row0mysql.h

@@ -576,7 +576,28 @@ struct row_prebuilt_t {
 					updated */
 	dtuple_t*	clust_ref;	/*!< prebuilt dtuple used in
 					sel/upd/del */
-	lock_mode	select_lock_type;/*!< LOCK_NONE, LOCK_S, or LOCK_X */
+	lock_mode	select_lock_type;/*!< LOCK_NONE, LOCK_S, LOCK_X, or
+					LOCK_X_BY_TABLE */
+
+	/** @return the lock mode to request for records and tables:
+	select_lock_type, with LOCK_X_BY_TABLE mapped to LOCK_X */
+	lock_mode real_lock_mode() const
+	{
+		return static_cast<lock_mode>(select_lock_type & LOCK_MODE_MASK);
+	}
+
+	/** @return the table lock to request for a locking read: LOCK_IS or
+	LOCK_IX, or the stronger LOCK_S or LOCK_X when full_table_scan makes
+	a single table lock replace the record locks.
+	select_lock_type must be LOCK_S, LOCK_X or LOCK_X_BY_TABLE. */
+	lock_mode table_lock_mode() const
+	{
+		ut_ad(select_lock_type == LOCK_S || select_lock_type == LOCK_X
+		      || select_lock_type == LOCK_X_BY_TABLE);
+		return static_cast<lock_mode>(
+			select_lock_type & (1U | unsigned{full_table_scan} << 1));
+	}
+
 	bool		skip_locked;	/*!< TL_{READ,WRITE}_SKIP_LOCKED */
 	lock_mode	stored_select_lock_type;/*!< this field is used to
 					remember the original select_lock_type

storage/innobase/row/row0mysql.cc

@@ -1194,8 +1194,7 @@ row_lock_table(row_prebuilt_t* prebuilt)
 
 		trx_start_if_not_started_xa(trx, false);
 
-		err = lock_table(prebuilt->table, NULL, static_cast<lock_mode>(
-					 prebuilt->select_lock_type), thr);
+		err = lock_table(prebuilt->table, NULL, prebuilt->real_lock_mode(), thr);
 		trx->error_state = err;
 	} while (err != DB_SUCCESS
 		 && row_mysql_handle_errors(&err, trx, thr, nullptr));
@@ -1833,8 +1832,7 @@ row_unlock_for_mysql(
 				trx,
 				*btr_pcur_get_block(pcur),
 				rec,
-				static_cast<enum lock_mode>(
-					prebuilt->select_lock_type));
+				prebuilt->real_lock_mode());
 		}
 no_unlock:
 		mtr_commit(&mtr);

storage/innobase/row/row0sel.cc

@@ -1352,6 +1352,8 @@ sel_set_rec_lock(
 		return DB_LOCK_TABLE_FULL;
 	}
 
+	/* Change LOCK_X_BY_TABLE to LOCK_X */
+	mode &= LOCK_MODE_MASK;
 	if (dict_index_is_clust(index)) {
 		err = lock_clust_rec_read_check_and_lock(
 			0, block, rec, index, offsets,
@@ -3387,7 +3389,7 @@ Row_sel_get_clust_rec_for_mysql::operator()(
 	/* Extract values from prebuilt */
 	dtuple_t*	clust_ref= prebuilt->clust_ref;
 	btr_pcur_t*	clust_pcur= prebuilt->clust_pcur;
-	lock_mode	select_lock_type= prebuilt->select_lock_type;
+	lock_mode	select_lock_type= prebuilt->real_lock_mode();
 	trx_t*		trx= prebuilt->trx;
 
 	dict_index_t*	clust_index;
@@ -4272,7 +4274,8 @@ bool row_search_with_covering_prefix(
 
 	/* In ha_innobase::build_template() we choose to access the
 	whole row when using exclusive row locks or In case of fts
-	query, we need to read from clustered index */
+	query, we need to read from clustered index. LOCK_X_BY_TABLE (see
+	ha_innobase::extra_opt()) is deliberately not matched here */
 	if (prebuilt->select_lock_type == LOCK_X || prebuilt->in_fts_query
 	    || !index->is_btree()) {
 		return false;
@@ -4828,10 +4831,7 @@ row_search_mvcc(
 		} else {
 wait_table_again:
 			err = lock_table(prebuilt->table, nullptr,
-                       prebuilt->full_table_scan ?
-                       prebuilt->select_lock_type :
-                       (prebuilt->select_lock_type == LOCK_S
-                        ? LOCK_IS : LOCK_IX), thr);
+                       prebuilt->table_lock_mode(), thr);
 
 			if (err != DB_SUCCESS) {