MDEV-37974 Avoid bogus deadlock in lock_rec_insert_check_and_lock() by arcivanov · Pull Request #4672 · MariaDB/server

arcivanov · 2026-02-20T06:37:23Z

When a transaction holds a granted lock on a record and another
transaction is waiting for that same record, an INSERT by the
lock-holding transaction into the gap before that record would
incorrectly enter lock_wait() on the waiting lock, creating a
false deadlock cycle.

In lock_rec_insert_check_and_lock(), after
lock_rec_other_has_conflicting() returns a conflicting lock,
check whether:

the conflicting lock is WAITING,
we hold a granted lock on the same record, and
no other transaction holds a GRANTED lock that conflicts
with our INSERT_INTENTION.

Only skip the lock wait when all three conditions are met. The scan
for granted conflicting locks is needed because lock inheritance
during purge can create granted GAP locks from other transactions
that coexist with our LOCK_ORDINARY but still block
INSERT_INTENTION.

The bogus deadlock in lock_delete_updated and versioning.update
(MDEV-14829 section) is also eliminated. In lock_delete_updated,
the DELETE no longer deadlocks but the row at the new PK position
is missed by the forward scan (pre-existing behavior). In
versioning.update, the concurrent UPDATE on a system-versioned
table no longer deadlocks because the historical row INSERT
correctly skips the waiting lock.

When a transaction holds a granted lock on a record and another transaction is waiting for that same record, an `INSERT` by the lock-holding transaction into the gap before that record would incorrectly enter `lock_wait()` on the waiting lock, creating a false deadlock cycle. In `lock_rec_insert_check_and_lock()`, after `lock_rec_other_has_conflicting()` returns a conflicting lock, check whether: 1. the conflicting lock is **WAITING**, 2. we hold a **granted** lock on the same record, and 3. no other transaction holds a **GRANTED** lock that conflicts with our `INSERT_INTENTION`. Only skip the lock wait when all three conditions are met. The scan for granted conflicting locks is needed because lock inheritance during purge can create granted `GAP` locks from other transactions that coexist with our `LOCK_ORDINARY` but still block `INSERT_INTENTION`. The bogus deadlock in `lock_delete_updated` and `versioning.update` (MDEV-14829 section) is also eliminated. In `lock_delete_updated`, the `DELETE` no longer deadlocks but the row at the new PK position is missed by the forward scan (pre-existing behavior). In `versioning.update`, the concurrent `UPDATE` on a system-versioned table no longer deadlocks because the historical row `INSERT` correctly skips the waiting lock.

arcivanov · 2026-02-22T20:04:28Z

@dr-m

gkodinov

Thank you for your contribution. This is a preliminary review. Please stand by for the final review.

dr-m

Thank you, this is interesting. But I think that we should be very careful here. We have been bitten by MDEV-27025 in the past. I think that we would need some additional testing in the style of https://mariadb.com/resources/blog/isolation-level-violation-testing-and-debugging-in-mariadb/ because our regular stress testing does not cover consistency or isolation very well.

dr-m · 2026-02-23T12:31:41Z

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

 --disable_query_log
 call mtr.add_suppression("InnoDB: Transaction was aborted due to ");
 --enable_query_log


Is any transaction being aborted anymore?

dr-m · 2026-02-23T12:32:47Z

mysql-test/suite/innodb/t/mdev_37974.opt

+--innodb-deadlock-detect=OFF
+--innodb-lock-wait-timeout=3


Why such a long timeout? Could we run two combinations of this test, for both values of innodb_deadlock_detect?

dr-m · 2026-02-23T12:34:09Z

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

+--connection con1
+--disconnect con1
+
+--connection default


The two --connection lines are redundant and should be removed.

dr-m · 2026-02-23T12:36:24Z

storage/innobase/lock/lock0lock.cc

+      DBUG_LOG("ib_lock",
+               "insert_check trx " << ib::hex(trx->id)
+               << " index " << index->name()
+               << " page " << id
+               << " heap_no " << heap_no);


If you think that such tracing is beneficial (I am too used to https://rr-project.org nowadays), I’d suggest to use DBUG_PRINT instead, to reduce the code footprint.

dr-m · 2026-02-23T12:40:02Z

storage/innobase/lock/lock0lock.cc

-        err= lock_rec_enqueue_waiting(c_lock, type_mode, id, block->page.frame,
-                                      heap_no, index, thr, nullptr);
-        trx->mutex_unlock();
+        lock_t *blocker= c_lock;


We don’t need a copy of this variable; we can just assign c_lock= nullptr; when there is no actual conflicting lock.

dr-m · 2026-02-23T12:55:48Z

storage/innobase/lock/lock0lock.cc

      if (lock_t *c_lock= lock_rec_other_has_conflicting(type_mode,
                                                         g.cell(), id,
                                                         heap_no, trx))
      {
-        trx->mutex_lock();
-        err= lock_rec_enqueue_waiting(c_lock, type_mode, id, block->page.frame,
-                                      heap_no, index, thr, nullptr);
-        trx->mutex_unlock();
+        lock_t *blocker= c_lock;
+
+        /* MDEV-37974: If the first conflicting lock is WAITING and
+        we hold a granted lock on the successor record, the waiting
+        lock is necessarily blocked behind our lock in the queue
+        (directly or via queue ordering) and can never be granted
+        while our lock exists.
+
+        However, we must also verify that no other transaction holds
+        a GRANTED lock that conflicts with our INSERT_INTENTION.
+        Such locks can arise from lock inheritance during purge
+        (e.g., an inherited X GAP lock that coexists with our
+        LOCK_ORDINARY but still blocks INSERT_INTENTION). Only when
+        all conflicting locks from other transactions are WAITING
+        can we safely skip the lock wait. */
+        if (c_lock->is_waiting() &&
+            lock_rec_has_expl(LOCK_X | LOCK_REC_NOT_GAP,
+                              g.cell(), id, heap_no, trx))
+        {
+          const bool is_supremum=
+            (heap_no == PAGE_HEAP_NO_SUPREMUM);
+          blocker= nullptr;
+          for (lock_t *l= lock_sys_t::get_first(g.cell(), id,
+                                                 heap_no);
+               l; l= lock_rec_get_next(heap_no, l))
+          {
+            if (l->trx != trx && !l->is_waiting() &&
+                lock_rec_has_to_wait(trx, type_mode, l,
+                                     is_supremum))
+            {
+              blocker= l;


I would not refer to MDEV tickets in source code comments, unless it is about some new feature or an open bug that is not expected to be fixed soon. Likewise, I would use descriptive test case names, instead of using a ticket number as a test case name.

Do we really need the for loop? The function lock_move_granted_locks_to_front() suggests that any conflicting waiting lock requests must after any non-waiting requests in the hash bucket chain. Hence, if the first conflicting lock that we find is a waiting request rather than a granted lock, we should already know that there is no actual conflict.

Basically, I am wondering if the following logic would be sufficient:

if (lock_t *c_lock= lock_rec_other_has_conflicting(type_mode, g.cell(), id, heap_no, trx)) if (!c_lock->is_waiting() { trx->mutex_lock(); err= lock_rec_enqueue_waiting(c_lock, type_mode, id, block->page.frame, heap_no, index, thr, nullptr); trx->mutex_unlock(); }

Note: I did not consider different type_mode yet.

arcivanov force-pushed the MDEV-37974 branch 3 times, most recently from 23599ab to d9e12c5 Compare February 20, 2026 07:25

arcivanov marked this pull request as draft February 20, 2026 08:16

arcivanov force-pushed the MDEV-37974 branch 2 times, most recently from c04ea78 to df10682 Compare February 20, 2026 08:20

gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Feb 20, 2026

arcivanov force-pushed the MDEV-37974 branch 4 times, most recently from 081a437 to 0171d8d Compare February 22, 2026 02:21

arcivanov force-pushed the MDEV-37974 branch from 0171d8d to a99f1e0 Compare February 22, 2026 02:27

arcivanov marked this pull request as ready for review February 22, 2026 05:02

gkodinov approved these changes Feb 23, 2026

View reviewed changes

dr-m reviewed Feb 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

MDEV-37974 Avoid bogus deadlock in lock_rec_insert_check_and_lock()#4672

MDEV-37974 Avoid bogus deadlock in lock_rec_insert_check_and_lock()#4672
arcivanov wants to merge 1 commit intoMariaDB:10.11from
arcivanov:MDEV-37974

arcivanov commented Feb 20, 2026 •

edited

Loading

Uh oh!

arcivanov commented Feb 22, 2026

Uh oh!

gkodinov left a comment

Uh oh!

dr-m left a comment

Uh oh!

dr-m Feb 23, 2026

Uh oh!

dr-m Feb 23, 2026

Uh oh!

dr-m Feb 23, 2026

Uh oh!

dr-m Feb 23, 2026

Uh oh!

dr-m Feb 23, 2026

Uh oh!

dr-m Feb 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

		--innodb-deadlock-detect=OFF
		--innodb-lock-wait-timeout=3

Uh oh!

Comments

Conversation

arcivanov commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arcivanov commented Feb 22, 2026

Uh oh!

gkodinov left a comment

Choose a reason for hiding this comment

Uh oh!

dr-m left a comment

Choose a reason for hiding this comment

Uh oh!

dr-m Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

dr-m Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

dr-m Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

dr-m Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

dr-m Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

dr-m Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

arcivanov commented Feb 20, 2026 •

edited

Loading