From b2843218f6df049407558d6364a2f0e2a0bd9c08 Mon Sep 17 00:00:00 2001 From: Dave Gosselin Date: Wed, 17 Jun 2026 11:25:22 -0400 Subject: [PATCH] MDEV-25964: Unexpected bypass of lock When an uncommitted transaction inserts rows into a table and another statement locks rows in the same table (SELECT ... FOR UPDATE) while computing a MIN or MAX, then: 1. In a Debug build, the server aborts on an assertion 2. In a Release build, the server returns wrong results These errors occur because, while reading a group of rows for computing a MAX, the transaction timeout error was swallowed. Under the scenario described above and captured in the new test at this commit, QUICK_GROUP_MIN_MAX_SELECT::next_max() emits a lock timeout error during QUICK_GROUP_MIN_MAX_SELECT::get_next() but the error was suppressed if we computed a MIN. Keep the MAX result when the MIN call succeeded but the MAX result failed, so the error propagates to the client. Now that we expect an error to propagate on error computing a MAX, drop the assertion. --- mysql-test/main/group_min_max_innodb.result | 28 ++++++++++++ mysql-test/main/group_min_max_innodb.test | 47 +++++++++++++++++++++ sql/opt_range.cc | 44 ++++++++++++++++--- 3 files changed, 113 insertions(+), 6 deletions(-) diff --git a/mysql-test/main/group_min_max_innodb.result b/mysql-test/main/group_min_max_innodb.result index 27656374aee38..a62c949d33f0d 100644 --- a/mysql-test/main/group_min_max_innodb.result +++ b/mysql-test/main/group_min_max_innodb.result @@ -440,3 +440,31 @@ SET GLOBAL innodb_lru_scan_depth= @lru_depth.save; set global innodb_stats_persistent= @innodb_stats_persistent_save; set global innodb_stats_persistent_sample_pages= @innodb_stats_persistent_sample_pages_save; +# +# Begin 10.11 tests +# +CREATE TABLE t1 (pk INT, a INT, b INT, PRIMARY KEY (pk), KEY (a,b)) ENGINE=InnoDB; +INSERT INTO t1 (pk,a) VALUES (1,11),(2,12),(3,13),(4,14); +connect con1,localhost,root,,; +DELETE FROM t1 WHERE pk <= 3; +connection default; +INSERT INTO t1 (pk) VALUES (5); +connection con1; +START TRANSACTION; +INSERT INTO t1 (pk) VALUES (6); +connection default; +SET @save_innodb_lock_wait_timeout= @@innodb_lock_wait_timeout; +SET innodb_lock_wait_timeout= 1; +EXPLAIN SELECT MIN(b), MAX(b), a FROM t1 GROUP BY a FOR UPDATE; +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE t1 range NULL a 10 NULL # Using index for group-by +SELECT MIN(b), MAX(b), a FROM t1 GROUP BY a FOR UPDATE; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction +SET innodb_lock_wait_timeout= @save_innodb_lock_wait_timeout; +connection con1; +COMMIT; +DROP TABLE t1; +disconnect con1; +# +# End 10.11 tests +# diff --git a/mysql-test/main/group_min_max_innodb.test b/mysql-test/main/group_min_max_innodb.test index 33a3a8888a5d8..51b3487077a0c 100644 --- a/mysql-test/main/group_min_max_innodb.test +++ b/mysql-test/main/group_min_max_innodb.test @@ -323,3 +323,50 @@ SET GLOBAL innodb_lru_scan_depth= @lru_depth.save; set global innodb_stats_persistent= @innodb_stats_persistent_save; set global innodb_stats_persistent_sample_pages= @innodb_stats_persistent_sample_pages_save; + +--echo # +--echo # Begin 10.11 tests +--echo # + +# Default client connection to the database. +CREATE TABLE t1 (pk INT, a INT, b INT, PRIMARY KEY (pk), KEY (a,b)) ENGINE=InnoDB; +INSERT INTO t1 (pk,a) VALUES (1,11),(2,12),(3,13),(4,14); + +# Start a new, separate client connection to the database called 'con1'. +--connect (con1,localhost,root,,) +--send +DELETE FROM t1 WHERE pk <= 3; + +# On the default connection, insert a value into the table. +--connection default +INSERT INTO t1 (pk) VALUES (5); + +# On 'con1', insert a value within a transaction and hold the transaction while +# on the default connection try to compute an aggregate. +--connection con1 +--reap + +START TRANSACTION; +INSERT INTO t1 (pk) VALUES (6); + +--connection default +SET @save_innodb_lock_wait_timeout= @@innodb_lock_wait_timeout; +SET innodb_lock_wait_timeout= 1; +# SELECT ... FOR UPDATE locks the rows for write operations and prevents other transactions +# from modifying or reading the selected rows until the transaction ends. +# This should lead to the error, not a crash. +--replace_column 9 # +EXPLAIN SELECT MIN(b), MAX(b), a FROM t1 GROUP BY a FOR UPDATE; +--error ER_LOCK_WAIT_TIMEOUT +SELECT MIN(b), MAX(b), a FROM t1 GROUP BY a FOR UPDATE; +SET innodb_lock_wait_timeout= @save_innodb_lock_wait_timeout; + +# Cleanup, commit the 'con1' transaction. +--connection con1 +COMMIT; +DROP TABLE t1; +--disconnect con1 + +--echo # +--echo # End 10.11 tests +--echo # diff --git a/sql/opt_range.cc b/sql/opt_range.cc index b042445a7aa1a..cb064b88eceab 100644 --- a/sql/opt_range.cc +++ b/sql/opt_range.cc @@ -15715,6 +15715,11 @@ int QUICK_GROUP_MIN_MAX_SELECT::get_next() */ if (have_min) { + /* + If next_min returns a handler error (HA_ERR_...), then that error + will be propagated to the caller of this method by setting result + at the bottom of this loop. + */ min_res= next_min(); if (min_res == 0) update_min_result(); @@ -15723,12 +15728,26 @@ int QUICK_GROUP_MIN_MAX_SELECT::get_next() if ((have_max && !have_min) || (have_max && have_min && (min_res == 0))) { + /* + Handler errors returned by next_max get treated in one of two ways: + 1. In a Debug build, the assertion in this block will cause a + crash (because max_res != 0). + 2. In a Release build, the handler error will be swallowed up + by the the result value update at the end of this loop. + There may be valid group of rows available but next_max() will + return a lock wait timeout error on a transaction conflict. + That must be propagated to the caller. + + next_min() and next_max() are not symmetrical with respect to + how they access the underlying table. next_min() may read the + index if there's an infix of constants from equality predicates + available (key_infix_len > 0) but next_max() makes no such check + Hence the lock wait timeout may be seen by next_max() but not by + next_min(). + */ max_res= next_max(); if (max_res == 0) update_max_result(); - /* If a MIN was found, a MAX must have been found as well. */ - DBUG_ASSERT((have_max && !have_min) || - (have_max && have_min && (max_res == 0))); } /* If this is just a GROUP BY or DISTINCT without MIN or MAX and there @@ -15740,7 +15759,16 @@ int QUICK_GROUP_MIN_MAX_SELECT::get_next() make_prev_keypart_map(real_key_parts), HA_READ_KEY_EXACT); + /* + 1. If we're computing a MIN only, then take its result. + 2. If we're computing a MAX only, then take its result. + 3. If we're computing both a MIN and a MAX, then take the MIN + result which swallows the MAX result in all cases... + */ result= have_min ? min_res : have_max ? max_res : result; + // ... 4. so check if there's a MAX error and return it if it exists. + if (result == 0 && have_max && max_res != 0) + result= max_res; } while (result == HA_ERR_KEY_NOT_FOUND || result == HA_ERR_END_OF_FILE); if (result == HA_ERR_KEY_NOT_FOUND) @@ -15797,11 +15825,15 @@ int QUICK_GROUP_MIN_MAX_SELECT::next_min() } /* + The reason that next_min has more complexity than next_max() is + for NULLs handling. In MariaDB, NULLs appear first in a result + and by default sort as less-than every other value. + If the min/max argument field is NULL, skip subsequent rows in the same group with NULL in it. Notice that: - - if the first row in a group doesn't have a NULL in the field, no row - in the same group has (because NULL < any other value), - - min_max_arg_part->field->ptr points to some place in 'record'. + - if the first row in a group doesn't have a NULL in the field, no row + in the same group has (because NULL < any other value), + - min_max_arg_part->field->ptr points to some place in 'record'. */ if (min_max_arg_part && min_max_arg_part->field->is_null()) {