MDEV-20707: Missing memory barrier in parallel replication error handler in wait_for_prior_commit()

revision-id: 673e253724979fd9fe43a4a22bd7e1b2c3a5269e
Author: Kristian Nielsen

Fix missing memory barrier in wait_for_commit.

The function wait_for_commit::wait_for_prior_commit() has a fast path where it
checks without locks if wakeup_subsequent_commits() has already been called.
This check was missing a memory barrier. The waitee thread does two writes to
variables `waitee' and `wakeup_error', and if the waiting thread sees the
first write it _must_ also see the second or incorrect behavior will occur.
This requires memory barriers between both the writes (release semantics) and
the reads (acquire semantics) of those two variables.

Other accesses to these variables are done under lock or where only one thread
will be accessing them, and can be done without barriers (relaxed semantics).

1 files changed, 12 insertions, 5 deletions

diff --git a/sql/sql_class.h b/sql/sql_class.h
index 3f732a76e53..36cb42c6314 100644
--- a/sql/sql_class.h
+++ b/sql/sql_class.h
@@ -20,6 +20,7 @@
 
 /* Classes in mysql */
 
+#include <atomic>
 #include "dur_prop.h"
 #include <waiting_threads.h>
 #include "sql_const.h"
@@ -2011,7 +2012,7 @@ struct wait_for_commit
   /*
     The LOCK_wait_commit protects the fields subsequent_commits_list and
     wakeup_subsequent_commits_running (for a waitee), and the pointer
-    waiterr and associated COND_wait_commit (for a waiter).
+    waitee and associated COND_wait_commit (for a waiter).
   */
   mysql_mutex_t LOCK_wait_commit;
   mysql_cond_t COND_wait_commit;
@@ -2025,8 +2026,14 @@ struct wait_for_commit
 
     When this is cleared for wakeup, the COND_wait_commit condition is
     signalled.
+
+    This pointer is protected by LOCK_wait_commit. But there is also a "fast
+    path" where the waiter compares this to NULL without holding the lock.
+    Such read must be done with acquire semantics (and all corresponding
+    writes done with release semantics). This ensures that a wakeup with error
+    is reliably detected as (waitee==NULL && wakeup_error != 0).
   */
-  wait_for_commit *waitee;
+  std::atomic<wait_for_commit *> waitee;
   /*
     Generic pointer for use by the transaction coordinator to optimise the
     waiting for improved group commit.
@@ -2061,7 +2068,7 @@ struct wait_for_commit
       Quick inline check, to avoid function call and locking in the common case
       where no wakeup is registered, or a registered wait was already signalled.
     */
-    if (waitee)
+    if (waitee.load(std::memory_order_acquire))
       return wait_for_prior_commit2(thd);
     else
     {
@@ -2089,7 +2096,7 @@ struct wait_for_commit
   }
   void unregister_wait_for_prior_commit()
   {
-    if (waitee)
+    if (waitee.load(std::memory_order_relaxed))
       unregister_wait_for_prior_commit2();
     else
       wakeup_error= 0;
@@ -2111,7 +2118,7 @@ struct wait_for_commit
       }
       next_ptr_ptr= &cur->next_subsequent_commit;
     }
-    waitee= NULL;
+    waitee.store(NULL, std::memory_order_relaxed);
   }
 
   void wakeup(int wakeup_error);