Vladislav Lesin added a comment - 2021-07-23 12:16 - edited The initial idea for the fix was to bypass lock_rec_convert_impl_to_expl() call if requested lock is not LOCK_REC_NOT_GAP in lock_clust_rec_read_check_and_lock() and lock_sec_rec_read_check_and_lock(). But after I implemented it there were some rpl tests failed. Before MDEV-16675 when lock_rec_convert_impl_to_expl() was invoked from lock_clust_rec_read_check_and_lock()/lock_sec_rec_read_check_and_lock(), and gap lock was acquired, explicit non-gap lock was created from implicit one, and then, after lock_rec_convert_impl_to_expl() call, explicit gap lock was created with lock_rec_lock() call. So, before MDEV-16675 , when gap lock was acquired for the record with implicit lock of the same transaction, two explicit locks were created: 1) non-gap lock was converted from implicit lock, and 2) gap lock created by lock_rec_lock(). It looks like implicit to explicit lock conversion is unnecessary for that case. But when I removed it, I got some rpl tests failed. Particularly binlog_encryption.rpl_parallel_innodb_lock_conflict failed here: # Create a group commit with INSERT and DELETE, in that order. # The bug was that while the INSERT's insert intention lock does not block # the DELETE, the DELETE's gap lock _does_ block the INSERT. This could cause # a deadlock on the slave. --connection con1 SET debug_sync='commit_after_release_LOCK_prepare_ordered SIGNAL master_queued1 WAIT_FOR master_cont1'; send INSERT INTO t4 VALUES (7, NULL); --connection server_1 SET debug_sync='now WAIT_FOR master_queued1'; --connection con2 SET debug_sync='commit_after_release_LOCK_prepare_ordered SIGNAL master_queued2'; send DELETE FROM t4 WHERE b <= 3; --connection server_1 SET debug_sync='now WAIT_FOR master_queued2'; SET debug_sync='now SIGNAL master_cont1'; --connection con1 REAP; --connection con2 REAP; SET debug_sync='RESET'; --save_master_pos --connection server_2 --source include/start_slave.inc --sync_with_master --source include/stop_slave.inc SELECT * FROM t4 ORDER BY a; (7, NULL) row was not found on slave. So, it looks like that explicit non-gap lock is necessary and used somehow, I don't understand how. When I left the same logic as it was before MDEV-16675 if gap lock is acquired, all tests passed.

Vladislav Lesin added a comment - 2021-07-26 13:38 - edited I found the error in my previous try to avoid implicit to explicit lock conversion if the transaction itself holds the implicit lock on the record, and gap lock is requested for the record. Pushed 10.6 and 10.3 versions for testing (bb-10.6- MDEV-26206 impl-gap-1 and bb-10.3 MDEV-26206 -impl-gap-1 branches). The fix also influence MDEV-14479 , which was fixed in 10.5.2, so the fixes for 10. [34] and 10. [56] differs. It should be taken into account during 10.4->10.5 merging.

Marko Mäkelä added a comment - 2021-07-30 05:59 The 10.3 version of this changes each return true in lock_rec_convert_impl_to_expl() to return !gap_lock; which refers to an added Boolean parameter. As far as I can tell, there is no need to change lock_rec_convert_impl_to_expl() at all, but instead only a need to change the callers: Each call if (lock_rec_convert_impl_to_expl(…, gap_lock)) can be replaced with if (gap_lock || lock_rec_convert_impl_to_expl(…)) and it should still be equivalent to the proposed fix. In the 10.6 version, it is slightly different, maybe something like the following: if ((gap_lock && !lock_table_has(…, LOCK_X)) || lock_rec_convert_impl_to_expl(…)) Either way, as far as I can tell, only the functions lock_sec_rec_read_check_and_lock() and lock_clust_rec_read_check_and_lock() would have to be changed. Maybe we could move the lock_table_has(…, LOCK_X) up a few callers in the call stack, and pass the BTR_NO_LOCKING_FLAG to each index operation if we are already holding an exclusive table lock?

Vladislav Lesin added a comment - 2021-08-10 12:33 - edited When I tried to move table lock checking from lock_rec_lock() to the higher level, my initial idea was to set prebuilt->select_lock_type = LOCK_NONE before row_search_mvcc() call, but it does not work because prebuilt->select_lock_type == LOCK_X it treated as "UPDATE" execution inside of row_search_mvcc(), and for "UPDATE" persistent cursor position must be stored. The other idea was to move the check inside of row_search_mvcc(). The corresponding patch table-locked.diff is attached. But some tests fail, so I need more time to understand the reason. As moving the check from lock_rec_lock() does not block MDEV-20605 , I delayed the fix until MDEV-20605 is ready for testing. The current 10.6 version contains the fix for lock_rec_convert_impl_to_expl() as Marko suggested in the previous comment, and it's under RQG testing.