Revert THD::THD(skip_global_sys_var_lock) argumentbb-10.3-svoj-MDEV-14984

Originally introduced by e972125f1 to avoid harmless wait for
LOCK_global_system_variables in a newly created thread, which creation was
initiated by system variable update.

At the same time it opens dangerous hole, when system variable update
thread already released LOCK_global_system_variables and ack_receiver
thread haven't yet completed new THD construction. In this case THD
constructor goes completely unprotected.

Since ack_receiver.stop() waits for the thread to go down, we have to
temporarily release LOCK_global_system_variables so that it doesn't
deadlock with ack_receiver.run(). Unfortunately it breaks atomicity
of rpl_semi_sync_master_enabled updates and makes them not serialized.

LOCK_rpl_semi_sync_master_enabled was introduced to workaround the above.
TODO: move ack_receiver start/stop into repl_semisync_master
enable_master/disable_master under LOCK_binlog protection?

Part of MDEV-14984 - regression in connect performance

8 files changed, 29 insertions, 29 deletions

diff --git a/sql/mysqld.cc b/sql/mysqld.cc
index 73dd7ce36af..4f5026fd3b5 100644
--- a/sql/mysqld.cc
+++ b/sql/mysqld.cc
@@ -941,6 +941,7 @@ PSI_mutex_key key_LOCK_relaylog_end_pos;
 PSI_mutex_key key_LOCK_thread_id;
 PSI_mutex_key key_LOCK_slave_state, key_LOCK_binlog_state,
   key_LOCK_rpl_thread, key_LOCK_rpl_thread_pool, key_LOCK_parallel_entry;
+PSI_mutex_key key_LOCK_rpl_semi_sync_master_enabled;
 PSI_mutex_key key_LOCK_binlog;
 
 PSI_mutex_key key_LOCK_stats,
@@ -1038,6 +1039,7 @@ static PSI_mutex_info all_server_mutexes[]=
   { &key_LOCK_rpl_thread_pool, "LOCK_rpl_thread_pool", 0},
   { &key_LOCK_parallel_entry, "LOCK_parallel_entry", 0},
   { &key_LOCK_ack_receiver, "Ack_receiver::mutex", 0},
+  { &key_LOCK_rpl_semi_sync_master_enabled, "LOCK_rpl_semi_sync_master_enabled", 0},
   { &key_LOCK_binlog, "LOCK_binlog", 0}
 };
 
diff --git a/sql/semisync_master.cc b/sql/semisync_master.cc
index 4e37e3af58d..f9fb4d9147d 100644
--- a/sql/semisync_master.cc
+++ b/sql/semisync_master.cc
@@ -352,7 +352,7 @@ Repl_semi_sync_master::Repl_semi_sync_master()
 
 int Repl_semi_sync_master::init_object()
 {
-  int result;
+  int result= 0;
 
   m_init_done = true;
 
@@ -362,6 +362,8 @@ int Repl_semi_sync_master::init_object()
   set_wait_point(rpl_semi_sync_master_wait_point);
 
   /* Mutex initialization can only be done after MY_INIT(). */
+  mysql_mutex_init(key_LOCK_rpl_semi_sync_master_enabled,
+                   &LOCK_rpl_semi_sync_master_enabled, MY_MUTEX_INIT_FAST);
   mysql_mutex_init(key_LOCK_binlog,
                    &LOCK_binlog, MY_MUTEX_INIT_FAST);
   mysql_cond_init(key_COND_binlog_send,
@@ -383,7 +385,7 @@ int Repl_semi_sync_master::init_object()
   }
   else
   {
-    result = disable_master();
+    disable_master();
   }
 
   return result;
@@ -421,7 +423,7 @@ int Repl_semi_sync_master::enable_master()
   return result;
 }
 
-int Repl_semi_sync_master::disable_master()
+void Repl_semi_sync_master::disable_master()
 {
   /* Must have the lock when we do enable of disable. */
   lock();
@@ -446,14 +448,13 @@ int Repl_semi_sync_master::disable_master()
   }
 
   unlock();
-
-  return 0;
 }
 
 void Repl_semi_sync_master::cleanup()
 {
   if (m_init_done)
   {
+    mysql_mutex_destroy(&LOCK_rpl_semi_sync_master_enabled);
     mysql_mutex_destroy(&LOCK_binlog);
     mysql_cond_destroy(&COND_binlog_send);
     m_init_done= 0;
diff --git a/sql/semisync_master.h b/sql/semisync_master.h
index de5e3240802..517175b5b06 100644
--- a/sql/semisync_master.h
+++ b/sql/semisync_master.h
@@ -23,6 +23,7 @@
 #include "semisync_master_ack_receiver.h"
 
 #ifdef HAVE_PSI_INTERFACE
+extern PSI_mutex_key key_LOCK_rpl_semi_sync_master_enabled;
 extern PSI_mutex_key key_LOCK_binlog;
 extern PSI_cond_key key_COND_binlog_send;
 #endif
@@ -365,7 +366,6 @@ public:
 */
 class Repl_semi_sync_master
   :public Repl_semi_sync_base {
- private:
   Active_tranx    *m_active_tranxs;  /* active transaction list: the list will
                                       be cleared when semi-sync switches off. */
 
@@ -491,8 +491,8 @@ class Repl_semi_sync_master
   /* Enable the object to enable semi-sync replication inside the master. */
   int enable_master();
 
-  /* Enable the object to enable semi-sync replication inside the master. */
-  int disable_master();
+  /* Disable the object to disable semi-sync replication inside the master. */
+  void disable_master();
 
   /* Add a semi-sync replication slave */
   void add_slave();
@@ -619,6 +619,8 @@ class Repl_semi_sync_master
   int before_reset_master();
 
   void check_and_switch();
+
+  mysql_mutex_t LOCK_rpl_semi_sync_master_enabled;
 };
 
 enum rpl_semi_sync_master_wait_point_t {
diff --git a/sql/semisync_master_ack_receiver.cc b/sql/semisync_master_ack_receiver.cc
index 607d4844658..81f494c9d34 100644
--- a/sql/semisync_master_ack_receiver.cc
+++ b/sql/semisync_master_ack_receiver.cc
@@ -185,8 +185,7 @@ static void init_net(NET *net, unsigned char *buff, unsigned int buff_len)
 
 void Ack_receiver::run()
 {
-  // skip LOCK_global_system_variables due to the 3rd arg
-  THD *thd= new THD(next_thread_id(), false, true);
+  THD *thd= new THD(next_thread_id());
   NET net;
   unsigned char net_buff[REPLY_MESSAGE_MAX_LENGTH];
 
diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc
index 63a6770f7d1..f4dab11bb42 100644
--- a/sql/session_tracker.cc
+++ b/sql/session_tracker.cc
@@ -322,6 +322,7 @@ bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf,
 
 void Session_sysvars_tracker::init(THD *thd)
 {
+  mysql_mutex_assert_owner(&LOCK_global_system_variables);
   DBUG_ASSERT(thd->variables.session_track_system_variables ==
               global_system_variables.session_track_system_variables);
   DBUG_ASSERT(global_system_variables.session_track_system_variables);
diff --git a/sql/sql_class.cc b/sql/sql_class.cc
index da04ddad49c..2eebfbb6db0 100644
--- a/sql/sql_class.cc
+++ b/sql/sql_class.cc
@@ -597,7 +597,7 @@ extern "C" void thd_kill_timeout(THD* thd)
   thd->awake(KILL_TIMEOUT);
 }
 
-THD::THD(my_thread_id id, bool is_wsrep_applier, bool skip_global_sys_var_lock)
+THD::THD(my_thread_id id, bool is_wsrep_applier)
   :Statement(&main_lex, &main_mem_root, STMT_CONVENTIONAL_EXECUTION,
              /* statement id */ 0),
    rli_fake(0), rgi_fake(0), rgi_slave(NULL),
@@ -792,7 +792,7 @@ THD::THD(my_thread_id id, bool is_wsrep_applier, bool skip_global_sys_var_lock)
   /* Call to init() below requires fully initialized Open_tables_state. */
   reset_open_tables_state(this);
 
-  init(skip_global_sys_var_lock);
+  init();
 #if defined(ENABLED_PROFILING)
   profiling.set_thd(this);
 #endif
@@ -1167,11 +1167,10 @@ const Type_handler *THD::type_handler_for_date() const
   Init common variables that has to be reset on start and on change_user
 */
 
-void THD::init(bool skip_lock)
+void THD::init()
 {
   DBUG_ENTER("thd::init");
-  if (!skip_lock)
-    mysql_mutex_lock(&LOCK_global_system_variables);
+  mysql_mutex_lock(&LOCK_global_system_variables);
   plugin_thdvar_init(this);
   /*
     plugin_thd_var_init() sets variables= global_system_variables, which
@@ -1184,8 +1183,7 @@ void THD::init(bool skip_lock)
   ::strmake(default_master_connection_buff,
             global_system_variables.default_master_connection.str,
             variables.default_master_connection.length);
-  if (!skip_lock)
-    mysql_mutex_unlock(&LOCK_global_system_variables);
+  mysql_mutex_unlock(&LOCK_global_system_variables);
 
   user_time.val= start_time= start_time_sec_part= 0;
 
diff --git a/sql/sql_class.h b/sql/sql_class.h
index e1119757957..221e453eab5 100644
--- a/sql/sql_class.h
+++ b/sql/sql_class.h
@@ -3263,17 +3263,12 @@ public:
   /**
     @param id                thread identifier
     @param is_wsrep_applier  thread type
-    @param skip_lock         instruct whether @c LOCK_global_system_variables
-                             is already locked, to not acquire it then.
   */
-  THD(my_thread_id id, bool is_wsrep_applier= false, bool skip_lock= false);
+  THD(my_thread_id id, bool is_wsrep_applier= false);
 
   ~THD();
-  /**
-    @param skip_lock         instruct whether @c LOCK_global_system_variables
-                             is already locked, to not acquire it then.
-  */
-  void init(bool skip_lock= false);
+
+  void init();
   /*
     Initialize memory roots necessary for query processing and (!)
     pre-allocate memory for it. We can't do that in THD constructor because
diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc
index 004fd4baecb..185078ff363 100644
--- a/sql/sys_vars.cc
+++ b/sql/sys_vars.cc
@@ -3144,6 +3144,8 @@ static Sys_var_replicate_events_marked_for_skip Replicate_events_marked_for_skip
 static bool fix_rpl_semi_sync_master_enabled(sys_var *self, THD *thd,
                                              enum_var_type type)
 {
+  mysql_mutex_unlock(&LOCK_global_system_variables);
+  mysql_mutex_lock(&repl_semisync_master.LOCK_rpl_semi_sync_master_enabled);
   if (rpl_semi_sync_master_enabled)
   {
     if (repl_semisync_master.enable_master() != 0)
@@ -3156,11 +3158,11 @@ static bool fix_rpl_semi_sync_master_enabled(sys_var *self, THD *thd,
   }
   else
   {
-    if (repl_semisync_master.disable_master() != 0)
-      rpl_semi_sync_master_enabled= true;
-    if (!rpl_semi_sync_master_enabled)
-      ack_receiver.stop();
+    repl_semisync_master.disable_master();
+    ack_receiver.stop();
   }
+  mysql_mutex_unlock(&repl_semisync_master.LOCK_rpl_semi_sync_master_enabled);
+  mysql_mutex_lock(&LOCK_global_system_variables);
   return false;
 }