Revert THD::THD(skip_global_sys_var_lock) argument

Originally introduced by e972125f 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

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}
 };
 

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;

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 @@ class Active_tranx
 */
 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 {

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];
 

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);

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;
 

sql/sql_class.h

@@ -3263,17 +3263,12 @@ class THD :public Statement,
   /**
     @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

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;
 }