Revert MDEV-18464 and MDEV-12009

This reverts commit 21b2fad
and commit 81d71ee.

The MDEV-18464 change introduces a few data race issues. Contrary to
the documentation, the field trx_t::victim is not always being protected
by lock_sys_t::mutex and trx_t::mutex. Most importantly, it seems
that KILL QUERY could wrongly avoid acquiring both mutexes when
invoking lock_trx_handle_wait_low(), in case another thread had
already set trx->victim=true.

We also revert MDEV-12009, because it should depend on the MDEV-18464
fix being present.

include/mysql/service_wsrep.h

@@ -112,7 +112,6 @@ extern struct wsrep_service_st {
   int                         (*wsrep_trx_order_before_func)(MYSQL_THD, MYSQL_THD);
   void                        (*wsrep_unlock_rollback_func)();
   void                        (*wsrep_set_data_home_dir_func)(const char *data_dir);
-  my_bool                     (*wsrep_thd_is_applier_func)(THD *thd);
 } *wsrep_service;
 
 #ifdef MYSQL_DYNAMIC_PLUGIN
@@ -156,7 +155,6 @@ extern struct wsrep_service_st {
 #define wsrep_trx_order_before(T1,T2) wsrep_service->wsrep_trx_order_before_func(T1,T2)
 #define wsrep_unlock_rollback() wsrep_service->wsrep_unlock_rollback_func()
 #define wsrep_set_data_home_dir(A) wsrep_service->wsrep_set_data_home_dir_func(A)
-#define wsrep_thd_is_applier(T) wsrep_service->wsrep_thd_is_applier_func(T)
 
 #define wsrep_debug get_wsrep_debug()
 #define wsrep_log_conflicts get_wsrep_log_conflicts()
@@ -216,7 +214,6 @@ void wsrep_thd_set_conflict_state(THD *thd, enum wsrep_conflict_state state);
 bool wsrep_thd_ignore_table(THD *thd);
 void wsrep_unlock_rollback();
 void wsrep_set_data_home_dir(const char *data_dir);
-my_bool wsrep_thd_is_applier(THD *thd);
 
 #endif
 

mysql-test/suite/galera/r/galera_kill_applier.result

@@ -1,8 +1,4 @@
-CREATE USER foo@localhost;
-GRANT SELECT on test.* TO foo@localhost;
-# Open connection to the 1st node using 'test_user1' user.
 Got one of the listed errors
 Got one of the listed errors
 Got one of the listed errors
 Got one of the listed errors
-DROP USER foo@localhost;

mysql-test/suite/galera/t/galera_kill_applier.test

@@ -1,74 +1,26 @@
 #
 # This test checks that applier threads are immune to KILL QUERY and KILL STATEMENT
-# when USER is not SUPER
 #
 
 --source include/galera_cluster.inc
+--source include/have_innodb.inc
 
 --connection node_1
 
-CREATE USER foo@localhost;
-GRANT SELECT on test.* TO foo@localhost;
-
 --let $applier_thread = `SELECT ID FROM INFORMATION_SCHEMA.PROCESSLIST WHERE USER = 'system user' AND STATE != 'wsrep aborter idle' OR STATE IS NULL LIMIT 1`
 
---let $aborter_thread = `SELECT ID FROM INFORMATION_SCHEMA.PROCESSLIST WHERE USER = 'system user' AND STATE = 'wsrep aborter idle' LIMIT 1`
-
---echo # Open connection to the 1st node using 'test_user1' user.
---let $port_1= \$NODE_MYPORT_1
---connect(foo_node_1,localhost,foo,,test,$port_1,)
-
---connection foo_node_1
-
 --disable_query_log
 --error ER_KILL_DENIED_ERROR,ER_KILL_DENIED_ERROR
 --eval KILL $applier_thread
 
 --error ER_KILL_DENIED_ERROR,ER_KILL_DENIED_ERROR
 --eval KILL QUERY $applier_thread
 
---error ER_KILL_DENIED_ERROR,ER_KILL_DENIED_ERROR
---eval KILL $aborter_thread
-
---error ER_KILL_DENIED_ERROR,ER_KILL_DENIED_ERROR
---eval KILL QUERY $aborter_thread
---enable_query_log
-
-#
-# SUPER can kill applier threads
-#
---connection node_2
-
---let $applier_thread = `SELECT ID FROM INFORMATION_SCHEMA.PROCESSLIST WHERE USER = 'system user' AND STATE != 'wsrep aborter idle' OR STATE IS NULL LIMIT 1`
-
---disable_query_log
---eval KILL $applier_thread
---enable_query_log
-
 --let $aborter_thread = `SELECT ID FROM INFORMATION_SCHEMA.PROCESSLIST WHERE USER = 'system user' AND STATE = 'wsrep aborter idle' LIMIT 1`
 
---disable_query_log
+--error ER_KILL_DENIED_ERROR,ER_KILL_DENIED_ERROR
 --eval KILL $aborter_thread
---enable_query_log
-
---source include/restart_mysqld.inc
 
---connection node_2
---let $applier_thread = `SELECT ID FROM INFORMATION_SCHEMA.PROCESSLIST WHERE USER = 'system user' AND STATE != 'wsrep aborter idle' OR STATE IS NULL LIMIT 1`
-
---disable_query_log
---eval KILL QUERY $applier_thread
---enable_query_log
-
---let $aborter_thread = `SELECT ID FROM INFORMATION_SCHEMA.PROCESSLIST WHERE USER = 'system user' AND STATE = 'wsrep aborter idle' LIMIT 1`
-
---disable_query_log
+--error ER_KILL_DENIED_ERROR,ER_KILL_DENIED_ERROR
 --eval KILL QUERY $aborter_thread
 --enable_query_log
-
---source include/restart_mysqld.inc
-
---connection node_1
---disconnect foo_node_1
-DROP USER foo@localhost;
-

sql/sql_parse.cc

@@ -8292,19 +8292,11 @@ kill_one_thread(THD *thd, longlong id, killed_state kill_signal, killed_type typ
       It's ok to also kill DELAYED threads with KILL_CONNECTION instead of
       KILL_SYSTEM_THREAD; The difference is that KILL_CONNECTION may be
       faster and do a harder kill than KILL_SYSTEM_THREAD;
-
-      Note that if thread is wsrep Brute Force or applier thread we
-      allow killing it only when we're SUPER.
     */
 
-    if ((thd->security_ctx->master_access & SUPER_ACL) ||
-	(thd->security_ctx->user_matches(tmp->security_ctx)
-#ifdef WITH_WSREP
-	 &&
-	 !tmp->wsrep_applier &&
-	 !wsrep_thd_is_BF(tmp, false)
-#endif
-	))
+    if (((thd->security_ctx->master_access & SUPER_ACL) ||
+        thd->security_ctx->user_matches(tmp->security_ctx)) &&
+	!wsrep_thd_is_BF(tmp, false))
     {
       tmp->awake(kill_signal);
       error=0;

sql/sql_plugin_services.ic

@@ -181,8 +181,7 @@ static struct wsrep_service_st wsrep_handler = {
   wsrep_trx_is_aborting,
   wsrep_trx_order_before,
   wsrep_unlock_rollback,
-  wsrep_set_data_home_dir,
-  wsrep_thd_is_applier,
+  wsrep_set_data_home_dir
 };
 
 static struct thd_specifics_service_st thd_specifics_handler=

sql/wsrep_dummy.cc

@@ -20,9 +20,6 @@
 my_bool wsrep_thd_is_BF(THD *, my_bool)
 { return 0; }
 
-my_bool wsrep_thd_is_applier(THD *)
-{ return 0; }
-
 int wsrep_trx_order_before(THD *, THD *)
 { return 0; }
 

sql/wsrep_mysqld.cc

@@ -2470,6 +2470,7 @@ extern "C" void wsrep_thd_set_exec_mode(THD *thd, enum wsrep_exec_mode mode)
   thd->wsrep_exec_mode= mode;
 }
 
+
 extern "C" void wsrep_thd_set_query_state(
 	THD *thd, enum wsrep_query_state state)
 {

sql/wsrep_thd.cc

@@ -596,15 +596,6 @@ my_bool wsrep_thd_is_BF(THD *thd, my_bool sync)
   return status;
 }
 
-my_bool wsrep_thd_is_applier(THD *thd)
-{
-  my_bool ret = FALSE;
-  if (thd) {
-      ret = thd->wsrep_applier;
-  }
-  return ret;
-}
-
 extern "C"
 my_bool wsrep_thd_is_BF_or_commit(void *thd_ptr, my_bool sync)
 {

sql/wsrep_thd.h

@@ -37,7 +37,6 @@ int  wsrep_abort_thd(void *bf_thd_ptr, void *victim_thd_ptr,
 */
 extern void  wsrep_thd_set_PA_safe(void *thd_ptr, my_bool safe);
 extern my_bool  wsrep_thd_is_BF(THD *thd, my_bool sync);
-extern my_bool wsrep_thd_is_applier(THD *thd);
 extern my_bool wsrep_thd_is_wsrep(void *thd_ptr);
 
 enum wsrep_conflict_state wsrep_thd_conflict_state(void *thd_ptr, my_bool sync);
@@ -48,7 +47,6 @@ extern "C" int  wsrep_thd_in_locking_session(void *thd_ptr);
 #else /* WITH_WSREP */
 
 #define wsrep_thd_is_BF(T, S) (0)
-#define wsrep_thd_is_applier(T) (0)
 #define wsrep_abort_thd(X,Y,Z) do { } while(0)
 #define wsrep_create_appliers(T) do { } while(0)
 

storage/innobase/handler/ha_innodb.cc

@@ -4929,6 +4929,8 @@ static void innobase_kill_query(handlerton*, THD* thd, enum thd_kill_levels)
 		/* if victim has been signaled by BF thread and/or aborting
 		   is already progressing, following query aborting is not necessary
 		   any more.
+		   Also, BF thread should own trx mutex for the victim, which would
+		   conflict with trx_mutex_enter() below
 		*/
 		DBUG_VOID_RETURN;
 	}
@@ -4937,8 +4939,34 @@ static void innobase_kill_query(handlerton*, THD* thd, enum thd_kill_levels)
 	if (trx_t* trx = thd_to_trx(thd)) {
 		ut_ad(trx->mysql_thd == thd);
 
+		switch (trx->abort_type) {
+#ifdef WITH_WSREP
+		case TRX_WSREP_ABORT:
+			break;
+#endif
+		case TRX_SERVER_ABORT:
+			if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
+				lock_mutex_enter();
+			}
+			/* fall through */
+		case TRX_REPLICATION_ABORT:
+			trx_mutex_enter(trx);
+		}
 		/* Cancel a pending lock request if there are any */
 		lock_trx_handle_wait(trx);
+		switch (trx->abort_type) {
+#ifdef WITH_WSREP
+		case TRX_WSREP_ABORT:
+			break;
+#endif
+		case TRX_SERVER_ABORT:
+			if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
+				lock_mutex_exit();
+			}
+			/* fall through */
+		case TRX_REPLICATION_ABORT:
+			trx_mutex_exit(trx);
+		}
 	}
 
 	DBUG_VOID_RETURN;
@@ -18655,12 +18683,6 @@ wsrep_innobase_kill_one_trx(
 		    wsrep_thd_ws_handle(thd)->trx_id);
 
 	wsrep_thd_LOCK(thd);
-
-	/* We mark this as victim transaction, which is already marked
-	as BF victim. Both trx mutex and lock_sys mutex is held until
-	this victim has aborted. */
-	victim_trx->victim = true;
-
         DBUG_EXECUTE_IF("sync.wsrep_after_BF_victim_lock",
                  {
                    const char act[]=
@@ -18844,7 +18866,7 @@ wsrep_abort_transaction(
 	my_bool signal)
 {
 	DBUG_ENTER("wsrep_innobase_abort_thd");
-
+	
 	trx_t* victim_trx	= thd_to_trx(victim_thd);
 	trx_t* bf_trx		= (bf_thd) ? thd_to_trx(bf_thd) : NULL;
 
@@ -18856,10 +18878,12 @@ wsrep_abort_transaction(
 	if (victim_trx) {
 		lock_mutex_enter();
 		trx_mutex_enter(victim_trx);
+		victim_trx->abort_type = TRX_WSREP_ABORT;
 		int rcode = wsrep_innobase_kill_one_trx(bf_thd, bf_trx,
                                                         victim_trx, signal);
 		trx_mutex_exit(victim_trx);
 		lock_mutex_exit();
+		victim_trx->abort_type = TRX_SERVER_ABORT;
 		wsrep_srv_conc_cancel_wait(victim_trx);
 		DBUG_RETURN(rcode);
 	} else {

storage/innobase/include/trx0trx.h

@@ -1,7 +1,7 @@
 /*****************************************************************************
 
 Copyright (c) 1996, 2016, Oracle and/or its affiliates. All Rights Reserved.
-Copyright (c) 2015, 2019, MariaDB Corporation.
+Copyright (c) 2015, 2018, MariaDB Corporation.
 
 This program is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free Software
@@ -623,6 +623,7 @@ struct trx_lock_t {
 					lock_sys->mutex. Otherwise, this may
 					only be modified by the thread that is
 					serving the running transaction. */
+
 	mem_heap_t*	lock_heap;	/*!< memory heap for trx_locks;
 					protected by lock_sys->mutex */
 
@@ -694,6 +695,14 @@ lock_rec_convert_impl_to_expl()) will access transactions associated
 to other connections. The locks of transactions are protected by
 lock_sys->mutex and sometimes by trx->mutex. */
 
+enum trx_abort_t {
+	TRX_SERVER_ABORT = 0,
+#ifdef WITH_WSREP
+	TRX_WSREP_ABORT,
+#endif
+	TRX_REPLICATION_ABORT
+};
+
 struct trx_t{
 	ulint		magic_n;
 
@@ -871,12 +880,8 @@ struct trx_t{
 	/*------------------------------*/
 	THD*		mysql_thd;	/*!< MySQL thread handle corresponding
 					to this trx, or NULL */
-	bool		victim;		/*!< This transaction is
-					selected as victim for abort
-					either by replication or
-					high priority wsrep thread. This
-					field is protected by trx and
-					lock sys mutex. */
+	trx_abort_t	abort_type;	/*!< Transaction abort type*/
+
 	const char*	mysql_log_file_name;
 					/*!< if MySQL binlog is used, this field
 					contains a pointer to the latest file