Problem

-------
The transaction dependency writeset history map doesn't
have protection from concurrent access.

Analysis
--------
This is because we are not locking m_writeset_history by
LOCK_slave_trans_dep_tracker at all places wherever it is
accessed whereas the same object is getting locked while
clearing the history map.

Fix
---
The Writeset_history::m_writeset_history is now guarded by
LOCK_slave_trans_dep_tracker where ever it is accessed and
this protects concurrent update on Writeset_history::
m_writeset_history.

Note : This bug fix also protects m_opt_tracking_mode variable
       explicitly using atomics.

RB#24250

Author: Kapil Agrawal

mysql-test/suite/binlog/r/binlog_writeset_history_protection.result

@@ -0,0 +1,15 @@
+CREATE TABLE t1 (i INT PRIMARY KEY);
+SET GLOBAL TRANSACTION_WRITE_SET_EXTRACTION = 'XXHASH64';
+SET GLOBAL BINLOG_TRANSACTION_DEPENDENCY_TRACKING = WRITESET;
+BEGIN;
+INSERT INTO t1 VALUES(1);
+SET DEBUG_SYNC="wait_in_get_dependency SIGNAL dep_tracker WAIT_FOR go_ahead_dml";
+commit;
+SET DEBUG_SYNC= "now WAIT_FOR dep_tracker";
+SET GLOBAL BINLOG_TRANSACTION_DEPENDENCY_TRACKING=WRITESET_SESSION;
+include/assert.inc [con2 is waiting for LOCK_slave_trans_dep_tracker.]
+SET DEBUG_SYNC="now SIGNAL go_ahead_dml";
+SET GLOBAL BINLOG_TRANSACTION_DEPENDENCY_TRACKING= TRACKING_MODE;
+SET GLOBAL TRANSACTION_WRITE_SET_EXTRACTION= EXTRACTION_ALG;
+SET DEBUG_SYNC="RESET";
+DROP TABLE t1;

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

@@ -0,0 +1,81 @@
+#=== Purpose ===
+#
+# This test verifies that the transaction dependency writeset
+# history map has protection from concurrent access.
+#
+# === Implementation ===
+#
+# 1. Do a DML on server such that it writes the GTID in the binlog and then halt
+#    the thread in get_dependency function using debug sync utility.
+# 2. Execute 'set global binlog_transaction_dependency_tracking=WRITESET or
+#    WRITESET_SESSION'.
+# 3. Verify that the thread in step 2 waits until LOCK_slave_trans_dep_tracker
+#    lock is released by the thread in step 1.
+#
+# === References ===
+#
+# Bug#29719364 THE BUGFIX FOR BUG#28511326 SEEMS INCORRECT
+#
+# This test case is binlog_format agnostic
+--source include/have_binlog_format_row.inc
+# This test case uses debug_sync
+--source include/have_debug.inc
+--source include/have_debug_sync.inc
+CREATE TABLE t1 (i INT PRIMARY KEY);
+
+# Save the current tracking mode and extraction algorithm.
+--let $old_trx_tracker = `SELECT @@GLOBAL.BINLOG_TRANSACTION_DEPENDENCY_TRACKING`
+--let $old_trx_write_set_extraction = `SELECT @@GLOBAL.TRANSACTION_WRITE_SET_EXTRACTION`
+
+# Set the global transaction extraction algorithm and tracking mode.
+SET GLOBAL TRANSACTION_WRITE_SET_EXTRACTION = 'XXHASH64';
+SET GLOBAL BINLOG_TRANSACTION_DEPENDENCY_TRACKING = WRITESET;
+
+--connect(con1,localhost,root,,)
+--connect(con2,localhost,root,,)
+
+# Do a DML such that it writes GTID and calls
+# get_dependency to update writeset history map.
+--connection con1
+BEGIN;
+INSERT INTO t1 VALUES(1);
+SET DEBUG_SYNC="wait_in_get_dependency SIGNAL dep_tracker WAIT_FOR go_ahead_dml";
+--send commit
+
+# Set transaction dependency tracking mode to writeset
+# so that it clears the writeset history.
+--connection con2
+--let $thread_id = `SELECT THREAD_ID FROM performance_schema.threads WHERE PROCESSLIST_ID =CONNECTION_ID()`
+SET DEBUG_SYNC= "now WAIT_FOR dep_tracker";
+--send SET GLOBAL BINLOG_TRANSACTION_DEPENDENCY_TRACKING=WRITESET_SESSION
+
+--connection default
+# wait until it tries to take LOCK_slave_trans_dep_tracker.
+--let $wait_condition= SELECT EVENT_NAME= 'wait/synch/mutex/sql/LOCK_slave_trans_dep_tracker' FROM performance_schema.events_waits_current WHERE THREAD_ID=$thread_id
+--source include/wait_condition.inc
+
+# since one thread has taken the LOCK_slave_trans_dep_tracker,
+# the other thread has to wait in order to aquire this lock.
+--connection default
+--let $assert_text= con2 is waiting for LOCK_slave_trans_dep_tracker.
+--let $assert_cond= "[SELECT EVENT_NAME FROM performance_schema.events_waits_current WHERE THREAD_ID=$thread_id]" = "wait/synch/mutex/sql/LOCK_slave_trans_dep_tracker"
+--source include/assert.inc
+
+#Signal the thread waiting in get_dependency.
+SET DEBUG_SYNC="now SIGNAL go_ahead_dml";
+
+--connection con1
+--reap
+--connection con2
+--reap
+
+#Cleanup
+--disconnect con1
+--disconnect con2
+--connection default
+--replace_result $old_trx_tracker TRACKING_MODE
+--eval SET GLOBAL BINLOG_TRANSACTION_DEPENDENCY_TRACKING= $old_trx_tracker
+--replace_result $old_trx_write_set_extraction EXTRACTION_ALG
+--eval SET GLOBAL TRANSACTION_WRITE_SET_EXTRACTION= $old_trx_write_set_extraction
+SET DEBUG_SYNC="RESET";
+DROP TABLE t1;

sql/binlog.cc

@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2020, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2009, 2020, Oracle and/or its affiliates.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -5249,7 +5249,10 @@ bool MYSQL_BIN_LOG::open_binlog(const char *log_name,
     At every rotate memorize the last transaction counter state to use it as
     offset at logging the transaction logical timestamps.
   */
+  mysql_mutex_lock(&LOCK_slave_trans_dep_tracker);
   m_dependency_tracker.rotate();
+  mysql_mutex_unlock(&LOCK_slave_trans_dep_tracker);
+
 #ifdef HAVE_REPLICATION
   close_purge_index_file();
 #endif

sql/mysqld.cc

@@ -1,4 +1,4 @@
-/* Copyright (c) 2000, 2019, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2000, 2020, Oracle and/or its affiliates.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -724,7 +724,6 @@ char *opt_general_logname, *opt_slow_logname, *opt_bin_logname;
 
 static volatile sig_atomic_t kill_in_progress;
 
-
 static my_bool opt_myisam_log;
 static int cleanup_done;
 static ulong opt_specialflag;
@@ -3092,6 +3091,12 @@ int init_common_variables()
     return 1;
   }
 
+  /*
+    We set the atomic field m_opt_tracking_mode to the value of the sysvar
+    variable m_opt_tracking_mode_value here, as it now has the user given
+    value.
+  */
+  set_mysqld_opt_tracking_mode();
   if (global_system_variables.transaction_write_set_extraction == HASH_ALGORITHM_OFF
       && mysql_bin_log.m_dependency_tracker.m_opt_tracking_mode != DEPENDENCY_TRACKING_COMMIT_ORDER)
   {
@@ -9587,4 +9592,9 @@ bool update_named_pipe_full_access_group(const char *new_group_name)
 
 #endif  /* _WIN32 && !EMBEDDED_LIBRARY */
 
+void set_mysqld_opt_tracking_mode()
+{
+  my_atomic_store64(&mysql_bin_log.m_dependency_tracker.m_opt_tracking_mode,
+  mysql_bin_log.m_dependency_tracker.m_opt_tracking_mode_value);
+}
 

sql/mysqld.h

@@ -1,4 +1,4 @@
-/* Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2010, 2020, Oracle and/or its affiliates.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -401,6 +401,11 @@ static inline int my_thread_set_THR_THD(THD *thd)
   return my_set_thread_local(THR_THD, thd);
 }
 
+/**
+  Set m_opt_tracking_mode with a user given value associated with sysvar.
+*/
+void set_mysqld_opt_tracking_mode();
+
 #ifdef HAVE_PSI_INTERFACE
 
 C_MODE_START

sql/rpl_trx_tracking.cc

@@ -1,4 +1,4 @@
-/* Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2018, 2020, Oracle and/or its affiliates.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -25,6 +25,7 @@
 #include "mysqld.h"
 #include "binlog.h"
 
+#include "debug_sync.h"
 
 Logical_clock::Logical_clock()
   : state(SEQ_UNINIT), offset(0)
@@ -217,6 +218,7 @@ Writeset_trx_dependency_tracker::get_dependency(THD *thd,
     !write_set_ctx->get_has_related_foreign_keys();
   bool exceeds_capacity= false;
 
+  mysql_mutex_lock(&LOCK_slave_trans_dep_tracker);
   if (can_use_writesets)
   {
     /*
@@ -231,6 +233,7 @@ Writeset_trx_dependency_tracker::get_dependency(THD *thd,
      Compute the greatest sequence_number among all conflicts and add the
      transaction's row hashes to the history.
     */
+    DEBUG_SYNC(thd, "wait_in_get_dependency");
     int64 last_parent= m_writeset_history_start;
     for (std::set<uint64>::iterator it= writeset->begin();
          it != writeset->end(); ++it)
@@ -271,6 +274,7 @@ Writeset_trx_dependency_tracker::get_dependency(THD *thd,
     m_writeset_history_start= sequence_number;
     m_writeset_history.clear();
   }
+  mysql_mutex_unlock(&LOCK_slave_trans_dep_tracker);
 }
 
 void
@@ -316,7 +320,7 @@ Transaction_dependency_tracker::get_dependency(THD *thd,
 {
   sequence_number= commit_parent= 0;
 
-  switch(m_opt_tracking_mode)
+  switch(my_atomic_load64(&m_opt_tracking_mode))
   {
     case DEPENDENCY_TRACKING_COMMIT_ORDER:
       m_commit_order.get_dependency(thd, sequence_number, commit_parent);

sql/rpl_trx_tracking.h

@@ -1,5 +1,5 @@
 #ifndef RPL_TRX_TRACKING_INCLUDED
-/* Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2018, 2020, Oracle and/or its affiliates.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -223,7 +223,10 @@ class Transaction_dependency_tracker
 
 public:
   /* option opt_binlog_transaction_dependency_tracking */
-  long m_opt_tracking_mode;
+  int64 m_opt_tracking_mode;
+
+  /* option opt_binlog_transaction_dependency_tracking associated with sysvar */
+  int64 m_opt_tracking_mode_value;
 
   Writeset_trx_dependency_tracker *get_writeset()
   {

sql/sys_vars.cc

@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2009, 2020, Oracle and/or its affiliates.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -3620,13 +3620,20 @@ static bool check_binlog_transaction_dependency_tracking(sys_var *self, THD *thd
 
 static bool update_binlog_transaction_dependency_tracking(sys_var* var, THD* thd, enum_var_type v)
 {
+  /*
+    m_opt_tracking_mode is read and written in an atomic way based
+    on the value of m_opt_tracking_mode_value that is associated
+    with the sys_var variable.
+  */
+    set_mysqld_opt_tracking_mode();
+
   /*
     the writeset_history_start needs to be set to 0 whenever there is a
     change in the transaction dependency source so that WS and COMMIT
     transition smoothly.
   */
-  mysql_bin_log.m_dependency_tracker.tracking_mode_changed();
-  return false;
+    mysql_bin_log.m_dependency_tracker.tracking_mode_changed();
+    return false;
 }
 
 static PolyLock_mutex
@@ -3639,7 +3646,7 @@ static Sys_var_enum Binlog_transaction_dependency_tracking(
        "assess which transactions can be executed in parallel by the "
        "slave's multi-threaded applier. "
        "Possible values are COMMIT_ORDER, WRITESET and WRITESET_SESSION.",
-       GLOBAL_VAR(mysql_bin_log.m_dependency_tracker.m_opt_tracking_mode),
+       GLOBAL_VAR(mysql_bin_log.m_dependency_tracker.m_opt_tracking_mode_value),
        CMD_LINE(REQUIRED_ARG), opt_binlog_transaction_dependency_tracking_names,
        DEFAULT(DEPENDENCY_TRACKING_COMMIT_ORDER),
        &PLock_slave_trans_dep_tracker,