MDEV-25902 Unexpected ER_LOCK_WAIT_TIMEOUT and result

trans_rollback_to_savepoint(): Only release metadata locks (MDL)
if the storage engines agree, after the changes were already rolled back.

Ever since commit 3792693f311a90cf195ec6d2f9b3762255a249c7
and mysql/mysql-server@55ceedbc3feb911505dcba6cee8080d55ce86dda
we used to cheat here and always release MDL if the binlog is disabled.

MDL are supposed to prevent race conditions between DML and DDL also
when no replication is in use. MDL are supposed to be a superset of
InnoDB table locks: InnoDB table lock may only exist if the thread
also holds MDL on the table name.

In the included test case, ROLLBACK TO SAVEPOINT would wrongly release
the MDL on both tables and let ALTER TABLE proceed, even though the DML
transaction is actually holding locks on the table.

Until commit 1bd681c8b3c5213ce1f7976940a7dc38b48a0d39 (MDEV-25506)
InnoDB worked around the locking violation in a blatantly non-ACID way:
If locks exist on a table that is being dropped (in this case, actually
a partition of a table that is being rebuilt by ALTER TABLE), InnoDB
would move the table (or partition) into a queue, to be dropped after
the locks and references had been released.

The scenario of commit 3792693f311a90cf195ec6d2f9b3762255a249c7
is unaffected by this fix, because mariadb-dump (a.k.a. mysqldump)
would use non-locking reads, and the transaction would not be holding
any InnoDB locks during the execution of ROLLBACK TO SAVEPOINT.
MVCC reads inside InnoDB are only covered by MDL and page latches,
not by any table or record locks.

FIXME: It would be nice if storage engines were specifically asked
which MDL can be released, instead of only offering a choice
between all or nothing. InnoDB should be able to release any
locks for tables that are no longer in trx_t::mod_tables, except
if another transaction had converted some implicit record locks
to explicit ones, before the ROLLBACK TO SAVEPOINT had been completed.

Reviewed by: Sergei Golubchik

3 files changed, 73 insertions, 29 deletions

diff --git a/mysql-test/suite/innodb/r/alter_partitioned.result b/mysql-test/suite/innodb/r/alter_partitioned.result
index cbdfab36499..ad6dd8b3e8c 100644
--- a/mysql-test/suite/innodb/r/alter_partitioned.result
+++ b/mysql-test/suite/innodb/r/alter_partitioned.result
@@ -16,3 +16,33 @@ CREATE TABLE t1 (id INT PRIMARY KEY, a INT, va INT AS (a) VIRTUAL)
 ENGINE=InnoDB PARTITION BY HASH(id) PARTITIONS 2;
 ALTER TABLE t1 ADD b INT, ALGORITHM=INSTANT;
 DROP TABLE t1;
+#
+# MDEV-25902 Unexpected ER_LOCK_WAIT_TIMEOUT and result,
+# assertion failure err != DB_DUPLICATE_KEY
+CREATE TABLE t1 (pk INT PRIMARY KEY) ENGINE=InnoDB;
+CREATE TABLE t2 (pk INT PRIMARY KEY) ENGINE=InnoDB;
+connect  con1,localhost,root,,test;
+START TRANSACTION;
+INSERT INTO t2 (pk) VALUES (1);
+SAVEPOINT sp;
+INSERT INTO t1 (pk) VALUES (1);
+ROLLBACK TO SAVEPOINT sp;
+connection default;
+SET lock_wait_timeout=0;
+SET innodb_lock_wait_timeout=0;
+ALTER TABLE t1 PARTITION BY HASH(pk);
+ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+SHOW CREATE TABLE t1;
+Table	Create Table
+t1	CREATE TABLE `t1` (
+  `pk` int(11) NOT NULL,
+  PRIMARY KEY (`pk`)
+) ENGINE=InnoDB DEFAULT CHARSET=latin1
+connection con1;
+COMMIT;
+connection default;
+ALTER TABLE t2 PARTITION BY HASH(pk);
+disconnect con1;
+connection default;
+DROP TABLE t1, t2;
+# End of 10.6 tests
diff --git a/mysql-test/suite/innodb/t/alter_partitioned.test b/mysql-test/suite/innodb/t/alter_partitioned.test
index e443f92bc8c..056791c6a0c 100644
--- a/mysql-test/suite/innodb/t/alter_partitioned.test
+++ b/mysql-test/suite/innodb/t/alter_partitioned.test
@@ -22,3 +22,36 @@ CREATE TABLE t1 (id INT PRIMARY KEY, a INT, va INT AS (a) VIRTUAL)
 ENGINE=InnoDB PARTITION BY HASH(id) PARTITIONS 2;
 ALTER TABLE t1 ADD b INT, ALGORITHM=INSTANT;
 DROP TABLE t1;
+
+--echo #
+--echo # MDEV-25902 Unexpected ER_LOCK_WAIT_TIMEOUT and result,
+--echo # assertion failure err != DB_DUPLICATE_KEY
+
+CREATE TABLE t1 (pk INT PRIMARY KEY) ENGINE=InnoDB;
+CREATE TABLE t2 (pk INT PRIMARY KEY) ENGINE=InnoDB;
+
+--connect (con1,localhost,root,,test)
+
+START TRANSACTION;
+INSERT INTO t2 (pk) VALUES (1);
+SAVEPOINT sp;
+INSERT INTO t1 (pk) VALUES (1);
+ROLLBACK TO SAVEPOINT sp;
+
+--connection default
+SET lock_wait_timeout=0;
+SET innodb_lock_wait_timeout=0;
+--error ER_LOCK_WAIT_TIMEOUT
+ALTER TABLE t1 PARTITION BY HASH(pk);
+
+SHOW CREATE TABLE t1;
+--connection con1
+COMMIT;
+--connection default
+ALTER TABLE t2 PARTITION BY HASH(pk);
+# Cleanup
+--disconnect con1
+--connection default
+DROP TABLE t1, t2;
+
+--echo # End of 10.6 tests
diff --git a/sql/transaction.cc b/sql/transaction.cc
index 8bd58419ec9..958abebfc47 100644
--- a/sql/transaction.cc
+++ b/sql/transaction.cc
@@ -1,5 +1,5 @@
 /* Copyright (c) 2000, 2013, Oracle and/or its affiliates. All rights reserved.
-   Copyright (c) 2009, 2020, MariaDB Corporation.
+   Copyright (c) 2009, 2021, MariaDB Corporation.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -667,33 +667,6 @@ bool trans_rollback_to_savepoint(THD *thd, LEX_CSTRING name)
   if (thd->transaction->xid_state.check_has_uncommitted_xa())
     DBUG_RETURN(TRUE);
 
-  /**
-    Checking whether it is safe to release metadata locks acquired after
-    savepoint, if rollback to savepoint is successful.
-  
-    Whether it is safe to release MDL after rollback to savepoint depends
-    on storage engines participating in transaction:
-  
-    - InnoDB doesn't release any row-locks on rollback to savepoint so it
-      is probably a bad idea to release MDL as well.
-    - Binary log implementation in some cases (e.g when non-transactional
-      tables involved) may choose not to remove events added after savepoint
-      from transactional cache, but instead will write them to binary
-      log accompanied with ROLLBACK TO SAVEPOINT statement. Since the real
-      write happens at the end of transaction releasing MDL on tables
-      mentioned in these events (i.e. acquired after savepoint and before
-      rollback ot it) can break replication, as concurrent DROP TABLES
-      statements will be able to drop these tables before events will get
-      into binary log,
-  
-    For backward-compatibility reasons we always release MDL if binary
-    logging is off.
-  */
-  bool mdl_can_safely_rollback_to_savepoint=
-                (!((WSREP_EMULATE_BINLOG_NNULL(thd) || mysql_bin_log.is_open())
-                 && thd->variables.sql_log_bin) ||
-                 ha_rollback_to_savepoint_can_release_mdl(thd));
-
   if (ha_rollback_to_savepoint(thd, sv))
     res= TRUE;
   else if (((thd->variables.option_bits & OPTION_KEEP_LOG) ||
@@ -705,7 +678,15 @@ bool trans_rollback_to_savepoint(THD *thd, LEX_CSTRING name)
 
   thd->transaction->savepoints= sv;
 
-  if (!res && mdl_can_safely_rollback_to_savepoint)
+  if (res)
+    /* An error occurred during rollback; we cannot release any MDL */;
+  else if (thd->variables.sql_log_bin &&
+           (WSREP_EMULATE_BINLOG_NNULL(thd) || mysql_bin_log.is_open()))
+    /* In some cases (such as with non-transactional tables) we may
+    choose to preserve events that were added after the SAVEPOINT,
+    delimiting them by SAVEPOINT and ROLLBACK TO SAVEPOINT statements.
+    Prematurely releasing MDL on such objects would break replication. */;
+  else if (ha_rollback_to_savepoint_can_release_mdl(thd))
     thd->mdl_context.rollback_to_savepoint(sv->mdl_savepoint);
 
   DBUG_RETURN(MY_TEST(res));