fix(lock): preserve bounded ranges for large DML batches - #26706
fix(lock): preserve bounded ranges for large DML batches#26706XuPeng-SH wants to merge 16 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
c24747a to
d0d2aef
Compare
9817e6c to
a151b4f
Compare
iamlinjunhong
left a comment
There was a problem hiding this comment.
Deep review on exact head 6f0ea624253f066266b0e83883ad11565de4a4c8 against PR base 4b62e3edd68729815ee4d982f6680bec35062408.
Requesting changes for three independent P1 correctness defects.
[P1] Re-entrant Exclusive rows can spuriously cross the budget and conflict with gap holders
coarsenLockRequest compares heldCount + len(rows) with the budget at pkg/lockservice/txn.go:256, even when incoming rows are exact keys already held by the transaction. Those requests add neither lock ownership nor local bookkeeping, so they must not force a wider representation.
Deterministic public-path counterexample with MaxLockRowCount=3: A holds Exclusive rows [1,5,9], B holds Exclusive row [3], then A re-locks its own row [1] with FastFail. The exact head rewrites this no-op acquisition to range [1,9] and returns ErrLockConflict against B. The same test passes at the merge base. With Wait policy the re-entrant request blocks instead. Incoming duplicates and fully re-entrant multi-row batches have the same counting problem.
The budget decision must be based on newly retained ownership, or at minimum bypass coarsening when the incoming request is already represented exactly. Add local and remote regressions with a foreign gap holder.
[P1] Remote deadlock snapshots lose the logical Shared-merge dependency and manufacture a self-cycle
The local traversal filters the physical self-edge of a Shared range-merge waiter using notifyOnSharedHolderChange and waitFor at pkg/lockservice/txn.go:760-763. The remote path does not preserve that rule: handleRemoteGetLock serializes every blocking waiter as w.txn at pkg/lockservice/service_remote.go:698-703, and remoteLockTable.doGetLock reconstructs each entry as a generic blocking edge at pkg/lockservice/lock_table_remote.go:456-464.
When A is physically queued on a Shared lock that A and B co-hold, the real dependency is A -> B. A deadlock traversal from another service asking who waits on A receives A itself and reports A -> A, aborting a valid request before B releases. The remote snapshot must apply the same requested-holder filter as the local traversal, or carry the logical waitFor metadata needed to reconstruct it.
[P1] A waiting cumulative replacement can discard locks acquired while its transaction mutex is released
service.Lock computes the replacement once at pkg/lockservice/service.go:288-304. A conflict then releases the transaction mutex at pkg/lockservice/lock_table_local.go:157-180; another lock call for the same transaction can add a key before the waiter reacquires that mutex. On wake, pkg/lockservice/lock_table_local.go:1010-1022 still commits replaceLocks from the stale pre-wait range and replaces all table bookkeeping, dropping the intervening key.
Concrete trigger with budget 4: A holds Exclusive [1,2,3], B holds [5], and A requests [5,6], preparing [1,6] and waiting on B. While blocked, A acquires row 9. After B unlocks, the stale replacement commits; A later unlocks only [1,6], leaving row 9 owned in the lock store, so another transaction FastFail on row 9 still conflicts. Recompute or generation-fence the replacement after every wait before publishing it; cover local, remote, and forwarded waits.
Validation: full production diff, ownership/wait-for/cleanup closure, planner fallback, proxy, and affected tests were inspected. The re-entrant counterexample fails deterministically at the exact head and passes unchanged at the merge base. The isolated checkout lacked native artifacts, so that differential used a test-only no-CGo allocator overlay; lockservice behavior was unmodified. All 26 current GitHub checks are completed with no failing conclusion.
|
Accepted all three latest P1 findings in The main reflection from this round is that transaction serialization is a critical-section invariant, not a whole-wait-lifecycle invariant. I initially tested a transaction-level lock-operation gate, but the full race suite exposed three existing same-transaction concurrency contracts that it would deadlock ( The final changes preserve those contracts:
Validation on the committed tree:
New CI has been triggered; this update does not wait for CI. |
iamlinjunhong
left a comment
There was a problem hiding this comment.
Requesting changes on exact head ae7875849d6a9846e3f2aaf03accacb7bb11bdb2. The three blockers from my previous review are addressed, but one new P1 liveness defect remains.
[P1] A Shared range-merge waiter's logical dependency set is frozen when it enters the queue. A compatible Shared transaction can join the same lock afterward, but local and remote deadlock snapshots filter through that stale set and omit the new holder. Deterministic public-path counterexample: A and B hold Shared row 1; A also holds an Exclusive row on another table; A requests Shared range [1,4] and queues with waitFor=[B]; C then acquires Shared row 1 and waits for A's Exclusive row. The real graph is A -> C -> A, but querying who waits on C drops A because C was not in the earlier snapshot. On the exact head no victim was selected within one second in 3/3 runs; both lock calls exited only after cancellation. With the production safety ceiling this can remain blocked for up to the configured lock-wait duration (one hour by default).
Please derive special-waiter edges from the current holder set, or update the logical dependency set whenever compatible holders join/leave, while continuing to exclude only the waiter's physical self-edge. Add a late-joining Shared-holder cycle regression for local and remote snapshots.
Validation: the focused tests for re-entrant distinct counting, remote snapshot self-edge filtering, and stale replacement preservation all pass on this head. The PR merges cleanly with current origin/main; all 26 reported checks are terminal with required checks green (remaining entries are expected skipped/neutral jobs).
aptend
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head. The previously reported locking, deadlock-graph, and concurrent replacement issues are addressed. Code review and focused race validation passed. LGTM.
|
Addressed the latest request-changes in The underlying defect was two competing sources of wait-for truth: the live lock holders and the enqueue-time The fix now enforces one snapshot invariant for both local traversal and the remote owner RPC:
Added deterministic coverage for:
Validation after the final semantic edit:
No lock acquisition/unlock hot path, lock ordering, wire format, or persistent format changed. The only added production cost is one O(1) holder-map lookup per candidate edge during a deadlock snapshot. |
aptend
left a comment
There was a problem hiding this comment.
Found one P1 correctness issue:
Repeated remote Exclusive range requests can make origin-side transaction bookkeeping grow past the cumulative budget. coarsenLockRequest deduplicates Row requests on the budget-crossing path, but deliberately returns Range endpoint pairs unchanged when the distinct-key set remains within the budget. The successful remote path then appends every multi-key request, even when the owner only replaced/reused the same physical range.
A concrete allowed configuration is MaxLockRowCount=2, MaxFixedSliceSize=4: requesting [1,2] twice fills the origin bookkeeping with four duplicate endpoints while the owner still has one pair. A subsequent row 3 request is coarsened to [1,3]. If the owner commits that replacement and the response is lost, the error path tries to retain the cleanup union, but lockAdded is already out of capacity and its error is ignored. The origin remains with stale [1,2] keys; unlock sees the owner range start at 1 but cannot find the stale end 2, so the committed [1,3] range is never released.
Please keep successful re-entrant Range bookkeeping aligned between owner and origin, and guarantee that the indeterminate-RPC cleanup union cannot fail silently. Add a regression covering a small valid capacity, repeated identical Range requests, a lost replacement response, and verification that unlock leaves no physical lock.
iamlinjunhong
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 2732cc7 against base 09e02ee.
[P1] Keep previously valid lock-capacity configurations bootable
pkg/lockservice/cfg.go:108-116 changes Config.Validate from accepting MaxLockRowCount <= MaxFixedSliceSize to requiring MaxLockRowCount <= roundUp(MaxFixedSliceSize)-2. This turns an existing accepted configuration into a startup panic during upgrade. A concrete example is max-row-lock-count=3 with max-fixed-slice-size=4: the merge base validates it successfully, while this head panics at line 116 with MaxFixedSliceSize must reserve two range endpoints beyond MaxLockRowCount. Equal power-of-two settings are affected in the same way. Any CN using such a tuned configuration will fail to start after the binary upgrade, before it can serve or migrate the setting.
Please preserve compatibility for configurations accepted by the previous release while still reserving safe cleanup capacity, for example through an explicit compatibility normalization/migration or a cleanup representation that does not require rejecting the old setting. Add a regression that loads a previously valid tight configuration across the upgrade boundary.
Validation: the differential Config.Validate test passes on the exact merge base and fails on this head. The latest local and remote late-joining Shared-holder deadlock regressions pass on this head, and all 26 GitHub checks are terminal with no failing conclusion. Local focused tests used a test-only no-CGo allocator overlay because this isolated host lacks the repository native artifacts; production lockservice code was unchanged.
ck89119
left a comment
There was a problem hiding this comment.
Deep re-review on exact head 2732cc7fff459b64f3528309ac41b05e92f15436.
The two blockers from my previous review are addressed: the waiting replacement now retains concurrent out-of-range ownership, and local/remote deadlock snapshots derive merge dependencies from live holders. The complete matrix found two independent P1 blockers that remain:
-
[P1] Revalidate coarsening eligibility when a waiting replacement commits.
coarsenLockRequestdecides that the table is all-Exclusive before the request sleeps, but the transaction mutex is released while waiting. A concurrent same-transaction Shared acquisition can permanently mark the table non-coarsenable; the old request still commits its Exclusive replacement and absorbs that Shared row. Public-path reproduction: A holds Exclusive rows 1/2/3, B holds Exclusive row 5, A waits on 5/6 as replacement [1,6], then A acquires Shared row 4. After B unlocks, C's Shared FastFail on row 4 incorrectly conflicts. This failed under race in local, remote-owner, and forwarding paths, 3/3 each. -
[P1] Make ordinary Shared range merge bookkeeping failure-atomic. For a non-budget replacement,
mc.commitdeletes the merged lock-store entries and removes their transaction keys before the followinglockAddedcan fail. WithMaxLockRowCount=6and effective fixed-slice capacity 8, A retains eight exact Shared rows and requests Shared [1,2]. The request returnsLockNeedUpgrade, but row 1 has already been removed; B can immediately acquire Exclusive row 1. Prepare the complete post-merge ledger before deleting the old representation, or roll back both ownership surfaces on failure.
Validation on this head:
- full
pkg/lockservice -race; - all 28 newly added lockservice tests under
-race -count=10; - full
pkg/sql/plan, plus its three new planner tests at-count=10; - full
pkg/sql/colexec/lockop -race; - affected-package
go buildand serialgo vet; - PR CI is green. Current
mo/mainis one unrelated frontend commit ahead; the merge-tree is conflict-free and does not touch affected packages.
Requesting changes until both failure matrices are closed with deterministic regressions.
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head 2732cc7. One P1 correctness blocker remains.
Repeated remote Exclusive range requests can grow origin-side bookkeeping beyond its fixed capacity. coarsenLockRequest deduplicates Row requests on the budget-crossing path, but returns explicit Range endpoint pairs unchanged when the distinct-key set is within budget (pkg/lockservice/txn.go:306-311). The remote success path then appends those endpoints, and its indeterminate-response cleanup path ignores lockAdded failure (pkg/lockservice/lock_table_remote.go:212).
With MaxLockRowCount=2 and MaxFixedSliceSize=4, requesting [1,2] twice fills origin bookkeeping with duplicate endpoints while the owner still has one range. A later replacement to [1,3] whose response is lost cannot retain the cleanup union once lockAdded is full; unlock can then leave the committed physical range behind. Please keep re-entrant Range bookkeeping aligned between owner and origin and make cleanup-union retention failure-atomic, with the small-capacity/lost-response regression.
|
@aptend @iamlinjunhong @ck89119 @aunjgr I accepted the latest request-changes and replaced the cleanup-union patching with one ownership model on rebased head The invariant is now:
This closes the current blockers as follows:
A further self-review covered heterogeneous rolling configuration: if the owner acknowledges success but the smaller origin cannot retain the detailed probe ledger, the origin still installs one bounded cleanup route instead of leaking owner state. No wire or persistent format changed. Ordinary local row acquisition gets no new scan; distinct-key compaction occurs only on the existing budget/capacity slow path, and remote-only markers add no local hot-path map allocation. Validation on the final rebased tree:
The previously unresolved late-holder inline thread was also replied to and resolved. Please re-review this head. |
aptend
left a comment
There was a problem hiding this comment.
Found one P1 correctness issue on head c6ef088a21bf1110a042a320f287a9718c3021d3:
When a waiting cumulative range replacement is invalidated by a concurrent Shared/sharded acquisition, the code rolls back the staged merge and switches c.rows/granularity back to the original Row request, but it does not detach the existing range waiter from rangeLastWaitKey. A successful Row fallback then only clears c.w; the old waiter remains at the head of an empty-holder lock queue with nobody left to consume it.
Concrete reproduction:
- A holds Exclusive rows 1/2/3, B holds gap row 5.
- A requests original row 6, which is coarsened to
[1,6]and waits on row 5. - While A waits, A acquires Shared row 4, invalidating coarsening eligibility.
- B unlocks; A falls back to exact row 6 and completes successfully.
- A's old waiter remains first on row 5. A later C request for row 5 cannot pass it: FastFail returns a spurious conflict and Wait mode remains blocked until timeout.
Please detach the old range waiter (queue ref, event/blocked state, wait edge, and rangeLastWaitKey) before changing the request to Row granularity, using the existing range-waiter cleanup path or an equivalent ownership-safe transition. Add local, remote, and forward regressions where the blocking key lies only in the coarsened gap and is absent from originalRows.
|
已合理采纳这条 P1,并在 根因不是 merge rollback 本身,而是 Range→Row 表示切换跨越了两个 wait generation:merge mutation 已回滚,但旧 range waiter 的 queue/blocked/wait-for ownership 没有在切换前结束。原测试把 blocker row 5 也放进 修复保持在唯一的 invalidation 慢路径: 回归覆盖 local、remote owner、forward:A 的原请求只有 row 6,先阻塞在 coarsened gap row 5;失效后又让 row 6 发生一次真实冲突,验证 waiter generation 重用,再断言 gap lock entry、owner-local edge、event checker 和 txn blocked state 全部为空,且第三方对 row 5 的 Exclusive FastFail 成功。未在普通 Row/Range 获取热路径增加扫描或分配。 验证:focused |
aptend
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head. The invalidated range waiter now closes its old queue, blocked-state, event-checker, and wait-for ownership before the exact-row retry, while preserving safe waiter generation reuse. The previous remote range bookkeeping, cleanup routing, coarsening eligibility, and atomic range-ledger blockers are also closed. Code review only; LGTM.
ck89119
left a comment
There was a problem hiding this comment.
Deep review on exact head 43df0e434537e6c83b1f8b9a6ef6f9594035692a against merge base 62f6bbbf4d9a1425dd95ab66f98671e839b22cf2.
Requesting changes for two PR correctness blockers and one directly in-scope proxy re-entry concurrency defect.
1. [P1] An ambiguous successful remote Exact acquisition loses deadlock-probe edges
On a transport error, ensureRemoteLockTableTracked retains only locks[:1]. That is sufficient to route the eventual transaction-ID unlock, but the same origin-side tableKeys ledger is also the only key set traversed by activeTxn.fetchWhoWaitingMe when constructing the distributed wait-for graph.
A successful re-entrant retry does not repair the ledger because the owner returns NewLockAdd=false, so remoteLockAdded is skipped. Deterministic two-service reproduction:
- The owner commits A's Exact rows
[1,2,3], and the first successful response is dropped. - The origin retains only row 1 as its cleanup witness.
- A retries the same request successfully.
- B waits on A's row 2 at the owner.
fetchWhoWaitingMe(A)at the origin returns an empty list instead of B.
The ledger regression passes on the merge base and fails on this head. Cleanup ownership is bounded, but deadlock termination is no longer complete. Please preserve complete probe coverage independently from the bounded cleanup route, or repair the origin ledger from the owner's authoritative representation on a successful retry. Add a lost-response + successful-retry regression that places a waiter on every non-witness Exact key and verifies the remote waiting-list snapshot.
Relevant paths: pkg/lockservice/lock_table_remote.go:194-252, pkg/lockservice/txn.go:353-381, and pkg/lockservice/txn.go:937-1000.
2. [P1] A low-estimate Shared batch is still converted to Range at runtime
applySharedLockTableFallback says underestimated Shared targets retain exact rows, and it only applies the planner fallback when Stats.Outcnt > MaxLockRowCount. However, both fixed and varlena fetchRows paths still unconditionally convert any actual batch larger than the threshold to [min,max] Range. lock_op then combines that granularity with Mode_Shared, and lockservice cannot reconstruct the original exact rows.
With MaxLockRowCount=3, let B hold Shared row 3 and let an underestimated A produce an actual batch spanning rows 1 through 5. Execution sends a Shared range [1,5]; FastFail returns ErrLockConflict against B, while Wait unnecessarily blocks on a logically compatible Shared holder. This contradicts the PR's stated low-estimate behavior and leaves the planner/runtime contract incomplete.
Please make runtime batch bounding mode-aware or retain the logical exact rows through the service boundary. Add execution-level fixed and varlena regressions where the estimate is below the threshold, the actual batch exceeds it, and a foreign Shared holder lies inside the derived range.
Relevant paths: pkg/sql/plan/build_util.go:46-73, pkg/sql/colexec/lockop/fetch.go:740-771,822-852, and pkg/sql/colexec/lockop/lock_op.go:641-665.
3. [P1] Concurrent same-transaction Shared proxy re-entry can deadlock and panic
The new proxy fast path deduplicates only after hasRemoteHolderLocked(key) becomes true. If two goroutines for the same activeTxn request the same singleton Shared row while the first remote RPC is in flight, the second appends the same transaction and waits while holding the transaction mutex. The first RPC completion must reacquire that mutex before running the proxy callback, creating a wait cycle. When the second request times out, sharedOps.remove(txn) removes both entries for the same transaction pointer; the first callback then indexes v.txns[0] and panics with index out of range at lock_table_proxy.go:132.
This defect is also reproducible at the merge base, so it is not a new head-vs-base regression. It is nevertheless directly inside the proxy re-entry path changed and tested by this PR: the added sequential repeated-lock test does not cover the in-flight generation. Please coalesce the same transaction before the remote holder is published, without waiting while holding its mutex, and make cancellation remove one request generation rather than every matching transaction entry.
Relevant paths: pkg/lockservice/lock_table_proxy.go:99-145,450-470.
Validation and readiness
- Full
pkg/lockservicepassed on the final head in 109.929s. - The new invalidated-range-waiter local/remote/forward regression passed at
-count=10. - All three deterministic counterexamples above fail on the final head; the first ledger differential passes on the merge base.
git diff --checkpassed.- Local race and full
lockop/planruns did not obtain test exits because the host exhausted disk and then exposed missing shared Go-cache archives; these are environment limitations, not recorded as code failures or passes. - Five GitHub checks are still in progress. The code verdict is independent of their eventual status.
The latest waiter-detach fix itself closes the previously reported queue/block/event/wait-edge ownership leak, but it does not affect the three blockers above.
Remove cardinality-driven implicit table-lock promotion. The lock operator already converts oversized inputs into bounded primary-key ranges, so retain row-scoped targets and allow disjoint key ranges to proceed concurrently. Explicit table locks remain unchanged.
Preserve complete remote deadlock probes after ambiguous RPCs, keep oversized Shared batches exact, and make same-transaction proxy reentry subscribe to one in-flight generation. Also close mixed proxy/direct unlock and range metadata cleanup edges found during full state-machine review.
Use owner-authoritative remote wait-for snapshots with rolling-upgrade fallback, reclaim empty proxy holder state, and preserve lock-table upgrades across retry generations.
Fixes #26630
Root cause
The planner previously promoted cardinality-estimated row locks to a full-domain table lock. That serialized disjoint DML, while simply removing the promotion left a second problem: execution splits a large DML into many sub-threshold lock calls, and a multi-statement transaction can retain more keys than any one call contains. The enforceable budget therefore belongs to the lock owner, per transaction and physical lock table.
Lock representation is also constrained by ownership semantics. Non-sharded Exclusive ownership can be conservatively replaced by the least observed
[min,max]range. Shared ownership cannot be cumulatively replaced the same way after acquisition because overlapping compatible holders are independent. Cardinality-known Shared targets therefore retain a planner-side pre-acquisition fallback, while runtime Shared ownership remains exact.Fix
FOR UPDATErow-scoped in the planner. At the authoritative lock owner, cumulatively coarsen only all-Exclusive, non-sharded ownership to the least observed range once distinct retained keys exceedMaxLockRowCount.SELECT FOR SHARE,LOCK IN SHARE MODE, and Shared FK validation.waitForset only seeds the initial detector check. This preserves dependencies on holders that join after enqueue, drops departed holders, and excludes only a Shared merge waiter's physical self-edge.The result is a bounded observed range rather than a table lock: gaps inside the range conflict conservatively, while keys outside its endpoints remain concurrent. No on-disk, wire, or checkpoint format changes are involved.
Regression coverage
FOR UPDATE, Shared lock syntax, and Shared FK validation.Validation
09e89b979bffb20df01bd83304e4b38c7e78e65e.4b62e3edd68729815ee4d982f6680bec35062408; conflict-free merge-tree againstorigin/main09e02eea38bfa87e4c4333ffb57b370091136bc8.pkg/lockservicenormal and race suites.-list.pkg/sql/planandpkg/sql/colexec/lockoptests.go buildandgo vetforpkg/lockservice,pkg/sql/plan, andpkg/sql/colexec/lockop.