From 8fe232256a22ee6842740710f40a5a12c942b678 Mon Sep 17 00:00:00 2001 From: Yuchen Pei Date: Wed, 21 Oct 2026 14:35:52 +1100 Subject: [PATCH] MDEV-41351 Allow a covering index scan for a SELECT ... FOR UPDATE under a full-scan table lock MDEV-24813 introduced a switch innodb_table_lock_on_full_scan that requests a table lock on an innodb table for some unconditional SELECT/UPDATE/DELETE statements. With a full table lock, and when doing a covering scan, there is no need to request clustered index reads. This patch does so for X-locks in SELECT statements. Prior to this it is already the case for S-locks. There is one exception: we disable this optimization when inside a transaction with innodb_snapshot_isolation and a read view already open, in which case it needs trx_id obtained from the clustered index to check if there is a committed change the transaction never saw. Assisted-By: Claude Opus 5 --- .../main/innodb_full_scan,table_lock.rdiff | 18 +++- mysql-test/main/innodb_full_scan.result | 78 +++++++++++++++ mysql-test/main/innodb_full_scan.test | 97 +++++++++++++++++++ storage/innobase/handler/ha_innodb.cc | 35 ++++++- storage/innobase/include/lock0priv.h | 1 + storage/innobase/include/lock0types.h | 7 +- storage/innobase/include/row0mysql.h | 23 ++++- storage/innobase/row/row0mysql.cc | 6 +- storage/innobase/row/row0sel.cc | 12 +-- 9 files changed, 259 insertions(+), 18 deletions(-) diff --git a/mysql-test/main/innodb_full_scan,table_lock.rdiff b/mysql-test/main/innodb_full_scan,table_lock.rdiff index 5bf2581eb6042..2523155d36e25 100644 --- a/mysql-test/main/innodb_full_scan,table_lock.rdiff +++ b/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; diff --git a/mysql-test/main/innodb_full_scan.result b/mysql-test/main/innodb_full_scan.result index d8eddbdfda22a..90794781fefaf 100644 --- a/mysql-test/main/innodb_full_scan.result +++ b/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; diff --git a/mysql-test/main/innodb_full_scan.test b/mysql-test/main/innodb_full_scan.test index 31eaf40d669ec..9a9c269caeb8b 100644 --- a/mysql-test/main/innodb_full_scan.test +++ b/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; diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 58af910e9db02..294f078c6e287 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/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); } diff --git a/storage/innobase/include/lock0priv.h b/storage/innobase/include/lock0priv.h index a8869f6db7247..d40addb475790 100644 --- a/storage/innobase/include/lock0priv.h +++ b/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"); diff --git a/storage/innobase/include/lock0types.h b/storage/innobase/include/lock0types.h index 39470b37ec921..978e746444124 100644 --- a/storage/innobase/include/lock0types.h +++ b/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 */ diff --git a/storage/innobase/include/row0mysql.h b/storage/innobase/include/row0mysql.h index 9e05f56318125..d51d8c2f6fe6e 100644 --- a/storage/innobase/include/row0mysql.h +++ b/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(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( + 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 diff --git a/storage/innobase/row/row0mysql.cc b/storage/innobase/row/row0mysql.cc index 39b15a6c2f72f..03ecc2e745623 100644 --- a/storage/innobase/row/row0mysql.cc +++ b/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( - 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( - prebuilt->select_lock_type)); + prebuilt->real_lock_mode()); } no_unlock: mtr_commit(&mtr); diff --git a/storage/innobase/row/row0sel.cc b/storage/innobase/row/row0sel.cc index 8f7e527d27101..88daa363a25fb 100644 --- a/storage/innobase/row/row0sel.cc +++ b/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) {