MDEV-37541 Race of rolling back and committing transaction to binlog

Two transactions could binlog their completions in opposite to how it
is done in Engine. That is is rare situations ROLLBACK in Engine of
the dependency parent transaction could be scheduled by the
transaction before its binlogging. That give a follower dependency
child one get binlogged ahead of the parent.

For fixing this bug its necessary to ensure the binlogging phase is
always first one in the internal one-phase rollback protocol.

The commit combines
1. a code polishing piece over a part of MDEV-21117 that
   made binlog handlerton always commit first in no-2pc cases and
2. the same rule now applies to the rollback.

An added test demonstrates how the child could otherwise reach binlog
before its parent.

Author: emoonrain

mysql-test/suite/binlog/t/binlog_mdev37541.test

@@ -2,6 +2,15 @@
 --source include/have_innodb.inc
 --source include/have_binlog_format_mixed.inc
 
+# MDEV-37541 Race of rolling back and committing transaction to binlog
+# The tests reproduce a scenario of two conflicting transactions
+# of which the dependency decides to rollback. In order to reproduce the bug
+# the parent features side effects (produced by DROP TEMPORARY TABLE)
+# and multiple engines.
+# The tests prove that the child transaction can not win race to binlogging
+# with its parent as it gets stuck at engine lock acquiring all time
+# while the parent conducts binlogging on its own.
+
 create table t1 (a int primary key, b text) engine=innodb;
 
 connect(trx1_rollback,localhost,root,,);

sql/handler.cc

@@ -2152,11 +2152,34 @@ static bool is_ro_1pc_trans(THD *thd, Ha_trx_info *ha_info, bool all,
   return !rw_trans;
 }
 
-static bool has_binlog_hton(Ha_trx_info *ha_info)
+inline Ha_trx_info* get_binlog_hton(Ha_trx_info *ha_info)
 {
-  bool rc;
-  for (rc= false; ha_info && !rc; ha_info= ha_info->next())
-    rc= ha_info->ht() == binlog_hton;
+  for (; ha_info; ha_info= ha_info->next())
+    if (ha_info->ht() == binlog_hton)
+      return ha_info;
+
+  return ha_info;
+}
+
+static int run_binlog_first(THD *thd, bool all, THD_TRANS *trans,
+                            bool is_real_trans, bool is_commit)
+{
+  int rc= 0;
+  Ha_trx_info *ha_info= trans->ha_list;
+
+  if ((ha_info= get_binlog_hton(ha_info)))
+  {
+    int err;
+    if ((err= is_commit ? binlog_commit(thd, all,
+                                        is_ro_1pc_trans(thd, ha_info, all,
+                                                        is_real_trans))
+                        : binlog_rollback(ha_info->ht(), thd, all)))
+    {
+      my_error(is_commit ? ER_ERROR_DURING_COMMIT : ER_ERROR_DURING_ROLLBACK,
+               MYF(0), err);
+      rc= 1;
+    }
+  }
 
   return rc;
 }
@@ -2174,18 +2197,19 @@ commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, bool is_real_trans)
   if (ha_info)
   {
     int err= 0;
-
-    if (has_binlog_hton(ha_info) &&
-        (err= binlog_commit(thd, all,
-                            is_ro_1pc_trans(thd, ha_info, all, is_real_trans))))
+    /*
+      Binlog hton must be called first regardless of its position
+      in trans->ha_list at least to prevent from commiting any engine
+      branches when afterward a duplicate GTID error out of binlog_commit()
+      is generated.
+    */
+    for (int binlog_err= error=
+           run_binlog_first(thd, all, trans, is_real_trans, true);
+         ha_info; ha_info= ha_info_next)
     {
-      my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
-      error= 1;
+      if (binlog_err)
+        goto err;
 
-      goto err;
-    }
-    for (; ha_info; ha_info= ha_info_next)
-    {
       handlerton *ht= ha_info->ht();
       if ((err= ht->commit(ht, thd, all)))
       {
@@ -2311,17 +2335,17 @@ int ha_rollback_trans(THD *thd, bool all)
     if (is_real_trans)                          /* not a statement commit */
       thd->stmt_map.close_transient_cursors();
 
-    int err;
-    if (has_binlog_hton(ha_info) &&
-        (err= binlog_hton->rollback(binlog_hton, thd, all)))
-    {
-      my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
-      error= 1;
-    }
-    for (; ha_info; ha_info= ha_info_next)
+    /*
+      Binlog hton must be called first regardless of its position
+      in trans->ha_list in order to avoid race to binlog between the current
+      rollbacker and any transaction that depends on it. This guarantees
+      the execution time dependency identifies binlog ordering.
+    */
+    for (error= run_binlog_first(thd, all, trans, is_real_trans, false);
+         ha_info; ha_info= ha_info_next)
     {
+      int err;
       handlerton *ht= ha_info->ht();
-
       if (ht != binlog_hton && (err= ht->rollback(ht, thd, all)))
       {
         // cannot happen

sql/log.cc

@@ -97,7 +97,6 @@ static int binlog_savepoint_set(handlerton *hton, THD *thd, void *sv);
 static int binlog_savepoint_rollback(handlerton *hton, THD *thd, void *sv);
 static bool binlog_savepoint_rollback_can_release_mdl(handlerton *hton,
                                                       THD *thd);
-static int binlog_rollback(handlerton *hton, THD *thd, bool all);
 static int binlog_prepare(handlerton *hton, THD *thd, bool all);
 static int binlog_start_consistent_snapshot(handlerton *hton, THD *thd);
 static int binlog_flush_cache(THD *thd, binlog_cache_mngr *cache_mngr,
@@ -2489,7 +2488,7 @@ int binlog_commit(THD *thd, bool all, bool ro_1pc)
 
   @see handlerton::rollback
 */
-static int binlog_rollback(handlerton *hton, THD *thd, bool all)
+int binlog_rollback(handlerton *hton, THD *thd, bool all)
 {
   DBUG_ENTER("binlog_rollback");
 

sql/log.h

@@ -1277,6 +1277,7 @@ const char *
 get_gtid_list_event(IO_CACHE *cache, Gtid_list_log_event **out_gtid_list);
 
 int binlog_commit(THD *thd, bool all, bool is_ro_1pc= false);
+int binlog_rollback(handlerton *hton, THD *thd, bool all);
 int binlog_commit_by_xid(handlerton *hton, XID *xid);
 int binlog_rollback_by_xid(handlerton *hton, XID *xid);
 bool write_bin_log_start_alter(THD *thd, bool& partial_alter,