MDEV-38465: Savepoint in trigger causes transactional inconsistency

SAVEPOINT inside a trigger doesn't work correctly. Setting a savepoint
inside a trigger somehow loses the implicit savepoint set at transaction
start, so that the partial changes are left if the statement later fails.
Referencing an existing savepoint claims the savepoint does not exist (and
it is in any case very unclear what exactly it should mean to rollback to a
savepoint from the middle of a statement, or set in the middle of a prior
statement).

These problems are independent of binlog-in-engine, but in the new binlog
implementation we are trying to make things work more correctly and
robustly, so let's disallow use of savepoints inside triggers. The new
binlog is off by default, so backwards compatibility is less of a concern,
though arguably disallowing savepoints in triggers would be better done
unconditionally.

Signed-off-by: Kristian Nielsen <knielsen@knielsen-hq.org>

Docs/replication/binlog.md

@@ -134,9 +134,13 @@ These are the chunk types used:
 
 ## Not supported
 
-A few things are not supported with the new binlog implementation. Some of these should be supported in a later version of MariaDB.
+A few things are not supported with the new binlog implementation. Some of
+these should be supported in a later version of MariaDB. Some of these are
+legacy stuff that fundamentally works poorly or is otherwise undesirable,
+and are intentionally removed in the new binlog implementation.
 
 - Old-style filename/offset replication positions are not available with the new binlog. Slaves must be configured to use GTID (this is the default). Event offsets are generally reported as zero. `MASTER_POS_WAIT()` is not available, `MASTER_GTID_WAIT()` should be used instead. Similarly, `BINLOG_GTID_POS()` is not available.
+- Using savepoints inside triggers is not supported. This is because of bugs and inconsistencies like in MDEV38465. Now executing a `SAVEPOINT` or `ROLLBACK TO SAVEPOINT` statement in a trigger will consistently error and roll back the entire statement.
 - Semi-synchronous replication is not supported in the first version. It will be supported as normal eventually using the `AFTER_COMMIT` option. The `AFTER_SYNC` option cannot be supported, as the expensive two-phase commit between binlog and engine is no longer needed (`AFTER_SYNC` waits for slave acknowledgement in the middle of the two-phase commit). Likewise, `--init-rpl-role` is not supported.
 - The new binlog implementation cannot be used with Galera.
 - In the initial version, only InnoDB is available as an engine for the binlog (`--binlog-storage-engine=innodb`). It the future, other transactional storage engines could implement storing the binlog themselves (performance is best when the binlog is implemented in the same engine as the tables that are updated).

mysql-test/suite/binlog_in_engine/not_implemented_yet.result

@@ -2,3 +2,22 @@ SET GLOBAL rpl_semi_sync_master_enabled= 1;
 ERROR HY000: Semi-synchronous replication is not yet supported with --binlog-storage-engine
 SELECT BINLOG_GTID_POS("binlog-000000.ibb", 4096);
 ERROR HY000: BINLOG_GTID_POS() is not available when --binlog-storage-engine is enabled
+CREATE TABLE t (a INT) ENGINE=InnoDB;
+CREATE TRIGGER tr AFTER INSERT ON t FOR EACH ROW BEGIN SAVEPOINT A; DELETE FROM non_existing; END $
+START TRANSACTION;
+INSERT INTO t VALUES (1), (3);
+ERROR HY000: Using savepoints in triggers is not available when --binlog-storage-engine is enabled
+COMMIT;
+SELECT * FROM t ORDER BY a;
+a
+DROP TRIGGER tr;
+CREATE TRIGGER tr AFTER INSERT ON t FOR EACH ROW BEGIN ROLLBACK TO SAVEPOINT B; DELETE FROM non_existing; END $
+START TRANSACTION;
+SAVEPOINT B;
+INSERT INTO t VALUES (11), (13);
+ERROR HY000: Using savepoints in triggers is not available when --binlog-storage-engine is enabled
+COMMIT;
+SELECT * FROM t ORDER BY a;
+a
+DROP TRIGGER tr;
+DROP TABLE t;

mysql-test/suite/binlog_in_engine/not_implemented_yet.test

@@ -8,3 +8,27 @@ SET GLOBAL rpl_semi_sync_master_enabled= 1;
 # want to get away from old problematic filename/offset coordinates.
 --error ER_NOT_AVAILABLE_WITH_ENGINE_BINLOG
 SELECT BINLOG_GTID_POS("binlog-000000.ibb", 4096);
+
+# MDEV-38465: Savepoint in trigger causes transactional inconsistency.
+CREATE TABLE t (a INT) ENGINE=InnoDB;
+--delimiter $
+CREATE TRIGGER tr AFTER INSERT ON t FOR EACH ROW BEGIN SAVEPOINT A; DELETE FROM non_existing; END $
+--delimiter ;
+START TRANSACTION;
+--error ER_NOT_AVAILABLE_WITH_ENGINE_BINLOG
+INSERT INTO t VALUES (1), (3);
+COMMIT;
+SELECT * FROM t ORDER BY a;
+DROP TRIGGER tr;
+
+--delimiter $
+CREATE TRIGGER tr AFTER INSERT ON t FOR EACH ROW BEGIN ROLLBACK TO SAVEPOINT B; DELETE FROM non_existing; END $
+--delimiter ;
+START TRANSACTION;
+SAVEPOINT B;
+--error ER_NOT_AVAILABLE_WITH_ENGINE_BINLOG
+INSERT INTO t VALUES (11), (13);
+COMMIT;
+SELECT * FROM t ORDER BY a;
+DROP TRIGGER tr;
+DROP TABLE t;

sql/transaction.cc

@@ -660,6 +660,20 @@ bool trans_savepoint(THD *thd, LEX_CSTRING name)
   if (thd->transaction->xid_state.check_has_uncommitted_xa())
     DBUG_RETURN(TRUE);
 
+  if (unlikely(thd->in_sub_stmt & SUB_STMT_TRIGGER) &&
+      opt_binlog_engine_hton)
+  {
+    /*
+      ToDo: Probably using savepoints in triggers should be disallowed
+      unconditionally. For now, we disallow it in the new binlog
+      implementation, which is not enabled by default, so that backwards
+      compatibility is of smaller concern.
+    */
+    my_error(ER_NOT_AVAILABLE_WITH_ENGINE_BINLOG, MYF(0),
+             "Using savepoints in triggers");
+    DBUG_RETURN(1);
+  }
+
   SAVEPOINT *newsv= savepoint_add(thd, Lex_ident_savepoint(name),
                                   &thd->transaction->savepoints,
                                   ha_release_savepoint);
@@ -713,10 +727,24 @@ bool trans_savepoint(THD *thd, LEX_CSTRING name)
 bool trans_rollback_to_savepoint(THD *thd, LEX_CSTRING name)
 {
   int res= FALSE;
-  SAVEPOINT *sv= *find_savepoint(thd, Lex_ident_savepoint(name));
+  SAVEPOINT *sv;
   DBUG_ENTER("trans_rollback_to_savepoint");
 
-  if (sv == NULL)
+  if (unlikely(thd->in_sub_stmt & SUB_STMT_TRIGGER) &&
+      opt_binlog_engine_hton)
+  {
+    /*
+      ToDo: Probably using savepoints in triggers should be disallowed
+      unconditionally. For now, we disallow it in the new binlog
+      implementation, which is not enabled by default, so that backwards
+      compatibility is of smaller concern.
+    */
+    my_error(ER_NOT_AVAILABLE_WITH_ENGINE_BINLOG, MYF(0),
+             "Using savepoints in triggers");
+    DBUG_RETURN(1);
+  }
+
+  if ((sv= *find_savepoint(thd, Lex_ident_savepoint(name))) == NULL)
   {
     my_error(ER_SP_DOES_NOT_EXIST, MYF(0), "SAVEPOINT", name.str);
     DBUG_RETURN(TRUE);