MDEV-29635 race on trx->lock.wait_lock in deadlock resolution

Returning DB_SUCCESS unconditionally if !trx->lock.wait_lock in
lock_trx_handle_wait() is wrong. Because even if
trx->lock.was_chosen_as_deadlock_victim was not set before the first check
in lock_trx_handle_wait(), it can be set after
the check, and trx->lock.wait_lock can be reset by another thread from
lock_reset_lock_and_trx_wait() if the transaction was chosen as deadlock
victim. In this case lock_trx_handle_wait() will return DB_SUCCESS even
the transaction was marked as deadlock victim, and continue execution
instead of rolling back.

The fix is to check trx->lock.was_chosen_as_deadlock_victim once more if
trx->lock.wait_lock is reset, as trx->lock.wait_lock can be reset only
after trx->lock.was_chosen_as_deadlock_victim was set if the transaction
was chosen as deadlock victim.

mysql-test/suite/innodb/r/deadlock_wait_lock_race.result

+connect suspend_purge,localhost,root,,;
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+connection default;
+CREATE TABLE t (a int PRIMARY KEY, b int) engine = InnoDB;
+CREATE TABLE t2 (a int PRIMARY KEY) engine = InnoDB;
+INSERT INTO t VALUES (10, 10), (20, 20), (30, 30);
+INSERT INTO t2 VALUES (10), (20), (30);
+BEGIN;
+UPDATE t2 SET a = a + 100;
+SELECT * FROM t WHERE a = 20 FOR UPDATE;
+a	b
+20	20
+connect con_2,localhost,root,,;
+SET TRANSACTION ISOLATION LEVEL READ COMMITTED;
+BEGIN;
+SET DEBUG_SYNC = 'lock_trx_handle_wait_before_unlocked_wait_lock_check SIGNAL upd_locked WAIT_FOR upd_cont';
+UPDATE t SET b = 100;
+connection default;
+SET DEBUG_SYNC="now WAIT_FOR upd_locked";
+SET DEBUG_SYNC="lock_wait_before_suspend SIGNAL upd_cont";
+SELECT * FROM t WHERE a = 10 FOR UPDATE;
+connection con_2;
+ERROR 40001: Deadlock found when trying to get lock; try restarting transaction
+disconnect con_2;
+connection default;
+a	b
+10	10
+SET DEBUG_SYNC = 'RESET';
+DROP TABLE t;
+DROP TABLE t2;
+disconnect suspend_purge;

mysql-test/suite/innodb/t/deadlock_wait_lock_race.test

+--source include/have_innodb.inc
+--source include/have_debug_sync.inc
+--source include/count_sessions.inc
+
+--connect(suspend_purge,localhost,root,,)
+# Purge can cause deadlock in the test, requesting page's RW_X_LATCH for trx
+# ids reseting, after trx 2 acqured RW_S_LATCH and suspended in debug sync point
+# lock_trx_handle_wait_enter, waiting for upd_cont signal, which must be
+# emitted after the last SELECT in this test. The last SELECT will hang waiting
+# for purge RW_X_LATCH releasing, and trx 2 will be rolled back by timeout.
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+
+--connection default
+CREATE TABLE t (a int PRIMARY KEY, b int) engine = InnoDB;
+CREATE TABLE t2 (a int PRIMARY KEY) engine = InnoDB;
+
+INSERT INTO t VALUES (10, 10), (20, 20), (30, 30);
+INSERT INTO t2 VALUES (10), (20), (30);
+
+BEGIN; # trx 1
+# The following update is necessary to increase the transaction weight, which is
+# calculated as the number of locks + the number of undo records during deadlock
+# report. Victim's transaction should have minimum weight. We need trx 2 to be
+# choosen as victim, that's why we need to increase the current transaction
+# weight.
+UPDATE t2 SET a = a + 100;
+SELECT * FROM t WHERE a = 20 FOR UPDATE;
+
+--connect(con_2,localhost,root,,)
+# RC is necessary to do semi-consistent read
+SET TRANSACTION ISOLATION LEVEL READ COMMITTED;
+BEGIN; # trx 2
+# The first time it will be hit on trying to lock (20,20), the second hit
+# will be on (30,30).
+SET DEBUG_SYNC = 'lock_trx_handle_wait_before_unlocked_wait_lock_check SIGNAL upd_locked WAIT_FOR upd_cont';
+# We must not modify primary key fields to cause rr_sequential() read record
+# function choosing in mysql_update(), i.e. both query_plan.using_filesort and
+# query_plan.using_io_buffer must be false during init_read_record() call.
+--send UPDATE t SET b = 100
+
+--connection default
+SET DEBUG_SYNC="now WAIT_FOR upd_locked";
+SET DEBUG_SYNC="lock_wait_before_suspend SIGNAL upd_cont";
+--send SELECT * FROM t WHERE a = 10 FOR UPDATE
+
+--connection con_2
+# If the bug is not fixed, lock_trx_handle_wait() wrongly returns DB_SUCCESS
+# instead of DB_DEADLOCK, row_search_mvcc() of trx 2 behaves so as if (20,20)
+# was locked. Some debug assertion must crash the server. If the bug is fixed,
+# trx 2 must just be rolled back by deadlock detector.
+--error ER_LOCK_DEADLOCK
+--reap
+
+--disconnect con_2
+
+--connection default
+--reap
+SET DEBUG_SYNC = 'RESET';
+DROP TABLE t;
+DROP TABLE t2;
+--disconnect suspend_purge
+--source include/wait_until_count_sessions.inc

storage/innobase/lock/lock0lock.cc

@@ -1822,6 +1822,7 @@ dberr_t lock_wait(que_thr_t *thr)
   while (trx->lock.wait_lock)
   {
     int err;
+    DEBUG_SYNC_C("lock_wait_before_suspend");
 
     if (no_timeout)
     {
@@ -5869,6 +5870,7 @@ lock_unlock_table_autoinc(
 
 /** Handle a pending lock wait (DB_LOCK_WAIT) in a semi-consistent read
 while holding a clustered index leaf page latch.
+
 @param trx           transaction that is or was waiting for a lock
 @retval DB_SUCCESS   if the lock was granted
 @retval DB_DEADLOCK  if the transaction must be aborted due to a deadlock
@@ -5879,8 +5881,13 @@ dberr_t lock_trx_handle_wait(trx_t *trx)
   DEBUG_SYNC_C("lock_trx_handle_wait_enter");
   if (trx->lock.was_chosen_as_deadlock_victim)
     return DB_DEADLOCK;
+  DEBUG_SYNC_C("lock_trx_handle_wait_before_unlocked_wait_lock_check");
+  /* trx->lock.was_chosen_as_deadlock_victim must always be set before
+  trx->lock.wait_lock if the transaction was chosen as deadlock victim,
+  the function must not return DB_SUCCESS if
+  trx->lock.was_chosen_as_deadlock_victim is set. */
   if (!trx->lock.wait_lock)
-    return DB_SUCCESS;
+    return trx->lock.was_chosen_as_deadlock_victim ? DB_DEADLOCK : DB_SUCCESS;
   dberr_t err= DB_SUCCESS;
   mysql_mutex_lock(&lock_sys.wait_mutex);
   if (trx->lock.was_chosen_as_deadlock_victim)
@@ -6283,6 +6290,8 @@ namespace Deadlock
 
       ut_ad(victim->state == TRX_STATE_ACTIVE);
 
+      /* victim->lock.was_chosen_as_deadlock_victim must always be set before
+      releasing waiting locks and reseting trx->lock.wait_lock */
       victim->lock.was_chosen_as_deadlock_victim= true;
       lock_cancel_waiting_and_release(victim->lock.wait_lock);
 #ifdef WITH_WSREP

storage/innobase/row/row0sel.cc

@@ -5270,6 +5270,8 @@ row_search_mvcc(
 
 			switch (err) {
 			case DB_SUCCESS:
+				ut_ad(
+				    !trx->lock.was_chosen_as_deadlock_victim);
 				/* The lock was granted while we were
 				searching for the last committed version.
 				Do a normal locking read. */