MDEV-40209 Escalate lock-release via a saturating stall counter#5299
MDEV-40209 Escalate lock-release via a saturating stall counter#5299iMineLink wants to merge 2 commits into
Conversation
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the optimistic lock-release paths in InnoDB by introducing a bounded spin budget (LOCK_RELEASE_TRY_SPIN_BUDGET) for per-cell and per-table latches, and replacing the fixed 5-pass retry limit with a progress-aware stall counter (LOCK_RELEASE_MAX_STALLS) and a hard ceiling (LOCK_RELEASE_MAX_PASSES). Feedback on the changes suggests that excluding the unlock_unmodified path from progress tracking in lock_release_on_prepare_try could lead to premature escalation to the exclusive lock_sys.wr_lock() under high concurrency. It is recommended to extend lock_rec_unlock_unmodified to track and report whether any locks were actually freed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR refines InnoDB lock-release “optimistic” paths to reduce unnecessary escalation from lock_sys.rd_lock() to the global lock_sys.wr_lock() under contention, aiming to prevent convoying when transactions are still making forward progress releasing locks.
Changes:
- Introduces bounded-spin trylock helpers for per-cell and per-table latch acquisition in lock-release fast paths.
- Replaces the fixed “5 failed passes then escalate” logic with a saturating stall counter plus a hard max-pass ceiling to better distinguish “stuck” from “slow but progressing”.
- Extends the
_tryhelpers to report per-pass progress to the callers without unlatched reads oftrx->lock.trx_locks.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The trylock attempts on per-cell lock_sys_t::hash_latch (try_acquire()) and on per-table dict_table_t::lock_mutex_trylock() inside lock_release_try(), lock_release_on_prepare_try() and lock_rec_unlock_unmodified() now use a bounded spin loop (up to LOCK_RELEASE_TRY_SPIN_BUDGET CAS attempts, with MY_RELAX_CPU() between them) instead of a single CAS attempt. These paths hold trx->mutex while attempting the trylock, which is the reverse of the standard order used by lock_rec_convert_impl_to_expl(). Blocking acquisition is therefore unsafe, hence the trylock pattern. However, a single failed CAS marks the entire pass of lock_release_try() as unsuccessful, and after 5 such failed passes lock_release() falls back to exclusive lock_sys.wr_lock() for the whole transaction. That global wr_lock then blocks every concurrent lock_sys.rd_lock() acquirer in lock_rec_lock() and lock_table(), producing a server-wide convoy under heavy concurrency. The bounded spin (no syscall, no blocking) gives a transient latch holder time to release without weakening the deadlock-avoidance guarantee that motivated the trylock pattern. The extra trx->mutex hold time is bounded by LOCK_RELEASE_TRY_SPIN_BUDGET times the pause cost. This is a first, still to be fine-tuned implementation. Only the lock_release_try() path has been positively tested; the lock_release_on_prepare_try() path is not yet covered.
lock_release() and lock_release_on_prepare() release a committing or preparing transaction's explicit locks under the shared lock_sys.rd_lock(), taking each per-cell hash latch and per-table lock mutex with a trylock because trx->mutex is held in the reverse of the normal latch order. A single failed trylock marked the whole pass unsuccessful, and after a fixed cap of 5 such passes the code escalated to the exclusive lock_sys.wr_lock() for the whole transaction. Under concurrency the trylocks fail transiently, so the cap escalated transactions that were still steadily releasing locks, not just stuck ones; the exclusive latch then blocks every concurrent lock_sys.rd_lock() acquirer in lock_rec_lock() and lock_table(), producing a convoy. The chance of hitting the cap rises with both the contention level and the number of latches a transaction must trylock per pass. Replace the fixed cap with a saturating stall counter (LOCK_RELEASE_MAX_STALLS, incremented on a no-progress pass, decremented on progress, floored at zero) that escalates a genuinely stuck transaction after 5 net stalls, as the fixed cap did, while leaving a transaction that keeps making progress to finish under the shared latch. A hard LOCK_RELEASE_MAX_PASSES ceiling bounds the loop independently, for the case where concurrent activity keeps adding locks (e.g. implicit-to-explicit conversion during XA PREPARE) so that progress never converges. The _try functions report progress through an out-parameter computed under trx->mutex, so trx->lock.trx_locks is never read unlatched.
|
I briefly checked the generated code for AMD64 and found no surprises. Performance testing is still needed to assess if the changes bring any benefit. |
Currently includes #5298, and not yet performance tested, so opened as a draft.