Marko Mäkelä added a comment - 2022-08-02 09:39 vlad.lesin , can you debug this? Or would this already be fixed by one of your recent changes? I used the following commands: ssh pluto rr replay /data/results/1643649542/TBR-1342/dev/shm/rqg/1643649542/10/1/rr/latest-trace continue frame 4 watch -l trx.lock.wait_lock.m._M_b._M_p reverse-continue reverse-continue thread apply 3 backtrace backtrace The assertion failed in Thread 3 for me. The lock had been replaced in Thread 39 executing ROLLBACK : bb-10.6-MDEV-20605-cur-pos-fix ac48095d5ba1c408cdc77d095985b2f20e46c82f #3 0x000055a9d9f24cda in Atomic_relaxed<ib_lock_t*>::operator= (this=0x7ffa5799fec0, i=0x7ffa579a0478) at /data/Server/bb-10.6-MDEV-20605-cur-pos-fix/include/my_atomic_wrapper.h:49 #4 0x000055a9d9ef8ca0 in lock_rec_create_low (c_lock=0x0, type_mode=259, page_id=..., page=0x7ffa49cf8000 "", heap_no=13, index=0x6160107b7208, trx=0x7ffa5799fe88, holds_trx_mutex=true, insert_before_waiting=false) at /data/Server/bb-10.6-MDEV-20605-cur-pos-fix/storage/innobase/lock/lock0lock.cc:1300 #5 0x000055a9d9efa572 in lock_rec_add_to_queue (type_mode=259, cell=..., id=..., page=0x7ffa49cf8000 "", heap_no=13, index=0x6160107b7208, trx=0x7ffa5799fe88, caller_owns_trx_mutex=true, insert_before_waiting=false) at /data/Server/bb-10.6-MDEV-20605-cur-pos-fix/storage/innobase/lock/lock0lock.cc:1521 #6 0x000055a9d9f00bfd in lock_rec_move (receiver_cell=..., receiver=..., receiver_id=..., donator_cell=..., donator_id=..., receiver_heap_no=13, donator_heap_no=0) at /data/Server/bb-10.6-MDEV-20605-cur-pos-fix/storage/innobase/lock/lock0lock.cc:2283 #7 0x000055a9d9f072a0 in lock_rec_restore_from_page_infimum (block=..., rec=0x7ffa49cf83fd "", donator=...) at /data/Server/bb-10.6-MDEV-20605-cur-pos-fix/storage/innobase/lock/lock0lock.cc:3187 #8 0x000055a9da2f40a5 in btr_cur_pessimistic_update (flags=7, cursor=0x61a0000e4178, offsets=0x100353764ee0, offsets_heap=0x100353764ec0, entry_heap=0x6190003ec680, big_rec=0x100353764d40, update=0x618000373d08, cmpl_info=0, thr=0x616004789cc0, trx_id=5395, mtr=0x100353764fc0) at /data/Server/bb-10.6-MDEV-20605-cur-pos-fix/storage/innobase/btr/btr0cur.cc:5134 #9 0x000055a9da5593eb in row_undo_mod_clust_low (node=0x61a0000e4108, offsets=0x100353764ee0, offsets_heap=0x100353764ec0, heap=0x6190003ec680, rebuilt_old_pk=0x100353764f00, sys=0x100353765500 "0UvS\003\020", thr=0x616004789cc0, mtr=0x100353764fc0, mode=33) at /data/Server/bb-10.6-MDEV-20605-cur-pos-fix/storage/innobase/row/row0umod.cc:142 At this time, the to-be-crashing Thread 3 was already executing on that assertion that would fail moments later: bb-10.6-MDEV-20605-cur-pos-fix ac48095d5ba1c408cdc77d095985b2f20e46c82f #0 lock_sys_t::cancel<false> (trx=0x7ffa5799fe88, lock=0x7ffa579a01c8) at /data/Server/bb-10.6-MDEV-20605-cur-pos-fix/storage/innobase/lock/lock0lock.cc:5744 #1 0x000055a9d9efd1e2 in lock_wait (thr=0x621000899db8) at /data/Server/bb-10.6-MDEV-20605-cur-pos-fix/storage/innobase/lock/lock0lock.cc:1918 #2 0x000055a9da10c759 in row_mysql_handle_errors (new_err=0x7ffa4d83c4c0, trx=0x7ffa5799fe88, thr=0x621000899db8, savept=0x0) at /data/Server/bb-10.6-MDEV-20605-cur-pos-fix/storage/innobase/row/row0mysql.cc:686 #3 0x000055a9da192e2c in row_search_mvcc (buf=0x61a00028c2b8 "\377", mode=PAGE_CUR_G, prebuilt=0x621000899588, match_mode=0, direction=0) at /data/Server/bb-10.6-MDEV-20605-cur-pos-fix/storage/innobase/row/row0sel.cc:5826 Because this is an unoptimized build, the read of trx->lock.wait_lock would occur in a separate procedure: => 0x000055a9d9f253ad <+387>: mov -0x28(%rbp),%rax 0x000055a9d9f253b1 <+391>: add $0x38,%rax 0x000055a9d9f253b5 <+395>: mov %rax,%rdi 0x000055a9d9f253b8 <+398>: callq 0x55a9d9dddea8 <Atomic_relaxed<ib_lock_t*>::operator ib_lock_t*() const> The modification by Thread 39 occurred between the first mov and the add . So, the value had already been modified by the time it was being read. The corresponding source code is: template < bool check_victim> dberr_t lock_sys_t::cancel(trx_t *trx, lock_t *lock) { mysql_mutex_assert_owner(&lock_sys.wait_mutex); ut_ad(trx->lock.wait_lock == lock); It is worth noting that Thread 39 was not holding lock_sys.wait_mutex at the time it updated the trx->lock.wait_lock . I may have wrongly assumed in lock_rec_create_low() that waiting locks are always being created for the transaction that is being executed in the current thread. It is definitely not the case here. This entire code in lock_rec_create_low() may need to be revised: if (type_mode & LOCK_WAIT) { if (trx->lock.wait_trx) { ut_ad(!c_lock || trx->lock.wait_trx == c_lock->trx); ut_ad(trx->lock.wait_lock); ut_ad((*trx->lock.wait_lock).trx == trx); } else { ut_ad(c_lock); trx->lock.wait_trx = c_lock->trx; ut_ad(!trx->lock.wait_lock); } trx->lock.wait_lock = lock; } When we are moving a waiting lock request of a ‘foreign’ thread, we must hold lock_sys.wait_mutex . Note: Starting with MDEV-24671 (10.6), lock_sys.wait_mutex must be acquired after lock_sys.latch . A possible fix would be to make all callers of lock_rec_move() check for the return value, which would indicate if any LOCK_WAIT records were encountered (and initially skipped). If they were, then we would have to retry the operation. Any code like the following would have to be adjusted: void lock_rec_restore_from_page_infimum( const buf_block_t &block, const rec_t *rec, page_id_t donator) { const ulint heap_no= page_rec_get_heap_no(rec); const page_id_t id{block.page.id()}; LockMultiGuard g{lock_sys.rec_hash, id, donator}; lock_rec_move(g.cell1(), block, id, g.cell2(), donator, heap_no, PAGE_HEAP_NO_INFIMUM); } In lock_rec_move() we could mysql_mutex_trylock(&lock_sys.wait_mutex) upon encountering the first LOCK_WAIT object. If it succeeds, fine. If not, the LockMultiGuard latches on the hash table cells must be released, lock_sys.wait_mutex acquired, and the hash table latches reacquired again.

Vladislav Lesin added a comment - 2023-01-18 08:22 The issue affects not only lock_rec_move(), but also lock_move_reorganize_page(), lock_move_rec_list_end(), lock_move_rec_list_start().

Vladislav Lesin added a comment - 2023-02-06 09:08 The solution, proposed by Marko above, does not work, because in the middle of locks moving locks queue becomes invalid, as some locks have already been moved, and some waiting locks don't have conflicting locks in the queue. If we release cell latch for such cell with invalid lock queue to acquire wait_lock, some other thread can add new lock to the queue, and as the queue is invalid, there can be wrong decision about transaction blocking necessity.

Marko Mäkelä added a comment - 2023-02-06 10:25 vlad.lesin , right, moving the locks must be atomic. Maybe I thought that the exclusive index tree leaf page latch would prevent any concurrent access to the index record locks on that page. I realize that when locks are released on transaction commit, no page latch would be acquired. How about the following? First, acquire LockMultiGuard latches and determine if lock_sys.wait_mutex needs to be acquired. If yes, and mysql_mutex_trylock(&lock_sys.wait_mutex) fails, release the latches (without having modified any locks yet), acquire them in the correct order. Finally, while holding the necessary latches, proceed to move the locks, and release the latches.

Vladislav Lesin added a comment - 2023-02-06 13:17 The issue is that trx->lock.wait_trx can be changed by other thread even if lock_sys.wait_mutex is held by the current thread. lock_sys.wait_mutex is held if something is added to or removed from waiting queues. Each queue at one hand belongs to the certain rec lock hash cell. At the other hand, each waiting transaction contains pointer to waited transaction, so we have some waiting graph, which is used to find deadlocks. The vertices of the graph are all active transactions, and the edges are dependencies. And lock_sys.wait_mutex is mostly used to keep the graph in consistent state to let deadlock detector to traverse the graph. Deadlock resolver uses Brent's cycle detection algorithm for each transaction in a global set, protected by lock_sys.wait_mutex. The other version of deadlock resolving function is one for the certain transaction instead of all transactions in the set. So, lock_sys.wait_mutex protects: 1. Deadlock::to_check set. 2. all trx->lock.wait_trx, i.e. the whole waiting graph 3. all trx->lock.wait_lock, which is atomic. Both wait_trx and wait_lock are changed in the same cases, i.e. either when wait_lock conflicts with some(granted or waiting) lock of wait_trx or when wait_lock is granted. 4. some statistics in lock_sys And lock_sys.wait_mutex is also used as a mutex for condition variable trx->lock.cond in lock_wait(), i.e. when some transaction is awakened after suspending on some record(or table) lock, wait_mutex is held by the transaction. Summarizing all the above, I see the following way to fix the bug. We could change locking order and acquire cell latches before lock_sys.wait_lock. In this case there is no need to reacquire cell latches, we can just acquire wait_mutex from lock_rec_create_low() when we want to change trx->lock.wait_lock. But this approach can potentially has negative performance impact. When we acquire separate cell latches instead of locking one global hash mutex, we distribute lock requests among hash table array elements, and such a way decrease the overall contention of the threads requesting the latches. If after some separate cell latch acquiring we are waiting for global lock_sys.wait_mutex, we increase the time during which the cell latch is held, increasing the probability of requesting the same cell latch by another thread. So overall contention on hash cell latches is also increased. I would say, we need performance testing after implementing this variant.

Matthias Leich added a comment - 2023-02-14 17:10 origin/bb-10.6- MDEV-27701 a876c4c303ae5942d30861edc6b8a14edbdc2db0 2023-02-14T10:46:37+03:00 performed well in RQG testing.