Merge MDEV-8725 into 10.0

mysql-test/suite/rpl/r/rpl_parallel.result

@@ -1689,6 +1689,24 @@ a	b
 include/stop_slave.inc
 SET GLOBAL debug_dbug=@old_dbug;
 include/start_slave.inc
+*** MDEV-8725: Assertion on ROLLBACK statement in the binary log ***
+BEGIN;
+INSERT INTO t2 VALUES (200);
+INSERT INTO t1 VALUES (200);
+INSERT INTO t2 VALUES (201);
+ROLLBACK;
+SELECT * FROM t1 WHERE a>=200 ORDER BY a;
+a
+200
+SELECT * FROM t2 WHERE a>=200 ORDER BY a;
+a
+include/save_master_gtid.inc
+include/sync_with_master_gtid.inc
+SELECT * FROM t1 WHERE a>=200 ORDER BY a;
+a
+200
+SELECT * FROM t2 WHERE a>=200 ORDER BY a;
+a
 include/stop_slave.inc
 SET GLOBAL slave_parallel_threads=@old_parallel_threads;
 include/start_slave.inc

mysql-test/suite/rpl/t/rpl_parallel.test

@@ -2369,6 +2369,28 @@ SET GLOBAL debug_dbug=@old_dbug;
 
 
 
+--echo *** MDEV-8725: Assertion on ROLLBACK statement in the binary log ***
+--connection server_1
+# Inject an event group terminated by ROLLBACK, by mixing MyISAM and InnoDB
+# in a transaction. The bug was an assertion on the ROLLBACK due to
+# mark_start_commit() being already called.
+--disable_warnings
+BEGIN;
+INSERT INTO t2 VALUES (200);
+INSERT INTO t1 VALUES (200);
+INSERT INTO t2 VALUES (201);
+ROLLBACK;
+--enable_warnings
+SELECT * FROM t1 WHERE a>=200 ORDER BY a;
+SELECT * FROM t2 WHERE a>=200 ORDER BY a;
+--source include/save_master_gtid.inc
+
+--connection server_2
+--source include/sync_with_master_gtid.inc
+SELECT * FROM t1 WHERE a>=200 ORDER BY a;
+SELECT * FROM t2 WHERE a>=200 ORDER BY a;
+
+
 # Clean up.
 --connection server_2
 --source include/stop_slave.inc

sql/log.cc

@@ -7660,14 +7660,13 @@ void
 MYSQL_BIN_LOG::binlog_trigger_immediate_group_commit()
 {
   group_commit_entry *head;
-  mysql_mutex_lock(&LOCK_prepare_ordered);
+  mysql_mutex_assert_owner(&LOCK_prepare_ordered);
   head= group_commit_queue;
   if (head)
   {
     head->thd->has_waiter= true;
     mysql_cond_signal(&COND_prepare_ordered);
   }
-  mysql_mutex_unlock(&LOCK_prepare_ordered);
 }
 
 
@@ -7686,9 +7685,11 @@ binlog_report_wait_for(THD *thd1, THD *thd2)
 {
   if (opt_binlog_commit_wait_count == 0)
     return;
+  mysql_mutex_lock(&LOCK_prepare_ordered);
   thd2->has_waiter= true;
   if (thd2->waiting_on_group_commit)
     mysql_bin_log.binlog_trigger_immediate_group_commit();
+  mysql_mutex_unlock(&LOCK_prepare_ordered);
 }
 
 

sql/rpl_parallel.cc

@@ -317,13 +317,26 @@ convert_kill_to_deadlock_error(rpl_group_info *rgi)
 }
 
 
-static bool
+/*
+  Check if an event marks the end of an event group. Returns non-zero if so,
+  zero otherwise.
+
+  In addition, returns 1 if the group is committing, 2 if it is rolling back.
+*/
+static int
 is_group_ending(Log_event *ev, Log_event_type event_type)
 {
-  return event_type == XID_EVENT ||
-         (event_type == QUERY_EVENT &&
-          (((Query_log_event *)ev)->is_commit() ||
-           ((Query_log_event *)ev)->is_rollback()));
+  if (event_type == XID_EVENT)
+    return 1;
+  if (event_type == QUERY_EVENT)
+  {
+    Query_log_event *qev = (Query_log_event *)ev;
+    if (qev->is_commit())
+      return 1;
+    if (qev->is_rollback())
+      return 2;
+  }
+  return 0;
 }
 
 
@@ -574,7 +587,7 @@ retry_event_group(rpl_group_info *rgi, rpl_parallel_thread *rpt,
       err= 1;
       goto err;
     }
-    if (is_group_ending(ev, event_type))
+    if (is_group_ending(ev, event_type) == 1)
       rgi->mark_start_commit();
 
     err= rpt_handle_event(qev, rpt);
@@ -713,7 +726,8 @@ handle_rpl_parallel_thread(void *arg)
       Log_event_type event_type;
       rpl_group_info *rgi= qev->rgi;
       rpl_parallel_entry *entry= rgi->parallel_entry;
-      bool end_of_group, group_ending;
+      bool end_of_group;
+      int group_ending;
 
       next_qev= qev->next;
       if (qev->typ == rpl_parallel_thread::queued_event::QUEUED_POS_UPDATE)
@@ -888,7 +902,18 @@ handle_rpl_parallel_thread(void *arg)
 
       group_rgi= rgi;
       group_ending= is_group_ending(qev->ev, event_type);
-      if (group_ending && likely(!rgi->worker_error))
+      /*
+        We do not unmark_start_commit() here in case of an explicit ROLLBACK
+        statement. Such events should be very rare, there is no real reason
+        to try to group commit them - on the contrary, it seems best to avoid
+        running them in parallel with following group commits, as with
+        ROLLBACK events we are already deep in dangerous corner cases with
+        mix of transactional and non-transactional tables or the like. And
+        avoiding the mark_start_commit() here allows us to keep an assertion
+        in ha_rollback_trans() that we do not rollback after doing
+        mark_start_commit().
+      */
+      if (group_ending == 1 && likely(!rgi->worker_error))
       {
         /*
           Do an extra check for (deadlock) kill here. This helps prevent a