|
The previous comment is wrong. The first, there is no need to push victim->lock.was_chosen_as_deadlock_victim set under trx_t::mutex_lock() protection. The second, finishing lock_cancel_waiting_and_release() if trx->lock.wait_thr == 0 is wrong too. The explanation is the following.
Suppose we have two transactions, trx 1 and trx 2 like in the issue description. trx 2 does deadlock resolving from lock_wait(), it set victim->lock.was_chosen_as_deadlock_victim= true for trx 1, but has not yet invoked lock_cancel_waiting_and_release().
trx 1 checked the flag in lock_trx_handle_wait(), and started rollback from row_mysql_handle_errors(). It can change trx->lock.wait_thr and trx->state as it holds trx_t::mutex, but trx 2 has not yet requested it, as lock_cancel_waiting_and_release() has not yet been called. Moreover, I suppose trx 1 intentionally resets trx->lock.wait_thr in trx_commit_or_rollback_prepare(), as there is some logic related to it in que_run_threads().
After that trx 1 tries to release locks in trx_t::rollback_low(), invoking trx_t::rollback_finish(). lock_release() is blocked on try to acquire lock_sys.rd_lock(SRW_LOCK_CALL) in lock_release_try(), as lock_sys is blocked by trx 2, as deadlock resolution works under lock_sys.wr_lock(SRW_LOCK_CALL), see Deadlock::report() for details.
trx 2 executes lock_cancel_waiting_and_release() for deadlock victim, i. e. for trx 1. lock_cancel_waiting_and_release() contains some trx->lock.wait_thr and trx->state assertions, which will fail, because trx 1 has changed them during rollback execution. But, suppose we commented out the assertions, and lock_cancel_waiting_and_release() continues execution without crash. What does it actually do? It dequeues victim waiting lock from queue in lock_rec_dequeue_from_page(), i. e. it removes it from queue, corresponding to (space,page), and then it traverses the queue. For each element in the queue it tries to find another conflicting element in the queue, and, if it is found, makes the first element to be waiting for the found element, i.e. it rebuilds waiting graph.
The most interesting thing in the above logic is that victim transaction can has another lock, conflicting, with the transaction which does deadlock resolving. And the procedure of waiting queue rebuilding will make the deadlock resolving transaction to be waiting for deadlock victim again. This is exactly what is happening in the test case: MDEV-29622.test.diff .
I.e. (20,20) record was locked by one transaction with SELECT FOR UPDATE, then (10,10) was locked by UPDATE of another transaction, but then the UPDATE created waiting lock for (20,20), as (20,20) was locked with conflicting lock by the first transaction. Then the first transaction executes SELECT FOR UPDATE again, tries to lock (10,10), detects deadlock and releases waiting lock of the UPDATE for (20,20). But the UPDATE still holds lock in (10,10), and the procedure of waiting queue rebuilding make the first transaction to be waiting for the UPDATE again.
So, trx 2 has just executed lock_cancel_waiting_and_release() and returned to lock_wait(). As trx 2 is still waiting for trx 1, trx 2 is suspended.
trx 1 acquires lock_sys mutex, releases its locks and wakes trx 2 up.
So, according to the above scenario, it's legal to have trx->lock.wait_thr==0 and trx->state!=TRX_STATE_ACTIVE in lock_cancel_waiting_and_release(), if it was invoked from Deadlock::report(), and the fix could be just in the assertions condition changing.
But why do we need to invoke lock_cancel_waiting_and_release(victim->lock.wait_lock) in Deadlock::report()? I suppose it's because the victim can be suspended in lock_wait(), and we need to wake it up to start rollback. Ok then, we can just wake it up with condition variable signalling(lock_wait_end(trx)), but we don't need to release the lock and rebuild waiting graph, it will be released in rollback, called from row_mysql_handle_errors().
|
MDEV-29622.test.diff