docs/design: add proposal for SELECT ... FOR UPDATE SKIP LOCKED#69783
docs/design: add proposal for SELECT ... FOR UPDATE SKIP LOCKED#69783takaidohigasi wants to merge 2 commits into
Conversation
Supersedes the unmerged 2022 proposal (pingcap#32625), redesigned on top of the modern pessimistic lock protocol (wake_up_mode, per-key PessimisticLockKeyResult): a new skip_locked request field with a LockResultSkipped per-key result, skip-on-KeyIsLocked-only semantics (write conflicts still retry), primary-key safety in client-go, and a three-layer compatibility rollout. ref pingcap#18207 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ti-chi-bot
Bot
added
the
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @takaidohigasi. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
needs-ok-to-test
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a TiDB design proposal for ChangesSkip-locked pessimistic reads
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/2026-07-12-select-for-update-skip-locked.md`:
- Around line 109-115: The skip_locked implementation must preserve
request-order mapping between mutations and per-key PessimisticLockKeyResult
entries, even when the TiKV path sorts mutations with ReturnValues disabled. Add
explicit original-index tracking or reorder results before returning, and add a
test covering unsorted multi-key input with mixed skipped/acquired statuses.
- Around line 109-115: Clarify the skip_locked contract in the design document
for non-skip errors occurring after earlier per-key results: specify whether the
server must return exactly one result per mutation by filling unprocessed
entries with LockResultFailed, or explicitly define client-go handling for
partial results alongside the error. Ensure the behavior applies to
WakeUpModeNormal and preserves request order.
- Around line 201-215: Update the upgrade-order and compatibility section of the
design to explicitly prevent TiKV downgrades below the minimum skip-locked
version, or enforce capability checks for every participating store before
issuing the request. Do not rely solely on the client-go response backstop,
since an incompatible store may block before returning; document the selected
enforcement mechanism and remove the claim that downgrades have no concern.
- Around line 184-188: Update the SelectLockExec skip-locked buffering design to
avoid unbounded child-row accumulation: implement a hard memory bound with
spill-to-disk or explicit rejection when the limit is reached, or revise the
documented execution strategy to guarantee bounded buffering. Ensure the
approach still filters rows after LockKeys with LockCtx.SkipLocked and remains
compatible with runPessimisticSelectForUpdate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 64aac78d-7019-4878-8d85-13687fb52098
📒 Files selected for processing (1)
docs/design/2026-07-12-select-for-update-skip-locked.md
| Contract: when `skip_locked = true`, `wake_up_mode` must be `WakeUpModeNormal`, and the | ||
| response carries exactly one `PessimisticLockKeyResult` per mutation (in request order) | ||
| in `results`; the legacy `values`/`not_founds` fields stay empty. `skip_locked` | ||
| together with `allow_lock_with_conflict`/`WakeUpModeForceLock` is rejected. Unlike | ||
| `WakeUpModeForceLock`, multi-key batches are allowed: since skipped keys neither wait | ||
| nor abandon the request, each key's disposition is independent (the same key-by-key | ||
| model as `AcquirePessimisticLockResumed`). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve request-order mapping for per-key results.
The contract requires results to be in request order, but the existing TiKV path sorts mutations when ReturnValues is false (pkg/store/mockstore/unistore/tikv/mvcc.go:237-239). Unless the new implementation preserves original indexes or restores the response order, skipped/acquired statuses can be applied to the wrong keys.
Define an explicit ordering mechanism and add a test with unsorted multi-key input.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/2026-07-12-select-for-update-skip-locked.md` around lines 109 -
115, The skip_locked implementation must preserve request-order mapping between
mutations and per-key PessimisticLockKeyResult entries, even when the TiKV path
sorts mutations with ReturnValues disabled. Add explicit original-index tracking
or reorder results before returning, and add a test covering unsorted multi-key
input with mixed skipped/acquired statuses.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Specify per-key results for partial failures.
The document promises exactly one result per mutation, but also says non-skip errors fail the request unchanged. If a WriteConflict occurs after earlier keys have produced results, the response may be incomplete. Existing mockstore completion logic only applies to WakeUpModeForceLock (pkg/store/mockstore/unistore/tikv/mvcc.go:225-230), while this design requires WakeUpModeNormal.
Clarify whether the server must fill remaining entries with LockResultFailed, or define how client-go handles partial results on errors.
Also applies to: 128-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/2026-07-12-select-for-update-skip-locked.md` around lines 109 -
115, Clarify the skip_locked contract in the design document for non-skip errors
occurring after earlier per-key results: specify whether the server must return
exactly one result per mutation by filling unprocessed entries with
LockResultFailed, or explicitly define client-go handling for partial results
alongside the error. Ensure the behavior applies to WakeUpModeNormal and
preserves request order.
- Make the request-order mapping of per-key results an explicit contract requirement (with unsorted multi-key test coverage). - Specify that request-level errors dominate partial results: the response carries errors, results stays empty, and clients process errors first and validate the results length otherwise. - Document the SelectLockExec buffer bound: it counts against tidb_mem_quota_query and triggers the configured OOM action instead of growing unboundedly; disk spilling noted as follow-up work. - Replace the "no downgrade concern" claim with explicit downgrade handling: per-statement min-version validation over all stores plus operational guidance to disable the feature before downgrades. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review comments in 63afa46:
|
1 participant
What problem does this PR solve?
Issue Number: ref #18207
Problem Summary:
SELECT ... FOR UPDATE SKIP LOCKED(MySQL 8.0.1+) is a long-requested feature (#18207,feature/accepted). An earlier proposal (#32625) was never merged and predates the modern pessimistic lock protocol (wake_up_mode, per-keyPessimisticLockKeyResult, resumable lock requests).What changed and how does it work?
Adds
docs/design/2026-07-12-select-for-update-skip-locked.md, a design proposal covering:KeyIsLocked); write conflicts on unlocked keys still fail and retry at a newerfor_update_ts(recomputing the skip set, matching MySQL's documented nondeterminism);FOR SHARE SKIP LOCKEDand multi-table joins out of scope for v1.PessimisticLockRequest.skip_lockedfield +LockResultSkippedper-key result type; per-key results inWakeUpModeNormal; multi-key batches allowed (skip never waits or abandons the request); incompatible withallow_lock_with_conflict.AcquirePessimisticLock(no clear-and-break, no lock-wait queue involvement); shared locks skipped without shrink-only conversion.LockCtx.SkipLocked+ skipped-key reporting; primary-key safety (a skipped primary with locked secondaries would let other transactions resolve those locks as rolled back, so candidate primaries are locked one by one);PessimisticRollbackKeysfor the unique-index atomic group.tidb_enable_select_skip_locked(default OFF, never silent no-op);SelectLockExecbuffer-lock-filter mode; PointGet/BatchPointGet skips with index-key lock rollback.A full working implementation across kvproto/TiKV/client-go/TiDB exists and passes the queue-worker pattern and skip semantics against a patched real TiKV cluster; implementation PRs will follow this design discussion. Preparatory bug-fix PR: #69782.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
SELECT ... FOR UPDATE SKIP LOCKEDin TiDB.