MDEV-25964: Unexpected bypass of lock

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
+#

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 #

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())
     {