fix(incrservice): avoid service lock during cache I/O - #27597
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? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
The main direction is correct: CurrentValue no longer performs GetColumns or initial allocation while holding service.mu, committed Delete now invalidates an in-flight generation, and Delete retirement is moved after the lock. However, the current head (68afae1b9172) still has three concurrency closure gaps, including one regression that can amplify the same slow-storage incident this PR is intended to contain.
-
[P1] Concurrent cold
CurrentValuecalls are not singleflight and each performs store I/O plus a persistent range allocation.acquireCommittedTableCachemisses underservice.muand every caller independently entersgetCommittedTableCacheForEpoch.generationBuildsis only an invalidation/publication fence; it does not give same-generation waiters one shared builder. All callers can therefore pass the post-GetColumnstable check before the firstnewTableCachefinishes, and everynewTableCacheperforms its own initialpreAllocate. Only one cache is eventually published, but all loser allocations have already advanced the persistent AUTO_INCREMENT offset and their ranges are abandoned. A deterministic 8-caller barrier test on this head observed exactly 8GetColumnscalls and 8Allocatecalls. Thus a cold burst turns one slow fileservice/store operation into N slow operations and can skip N allocation ranges, which is a serious regression from the old serialized path. Please add a per-table/per-generation in-flight build record so same-generation callers share one build/result while retaining independent waiter cancellation. Delete, Reload, and Close must invalidate/wake all waiters; a failed or canceled owner must not leave a stuck generation, and a later call must be able to rebuild. -
[P1] The any-epoch
CurrentValuecontract is only implemented in the first fast-path check and races with epoch-aware publication. The function can observe no cache, unlock, and then callgetCommittedTableCacheForEpoch(..., 0, nil). If anInsertValues/GetLastAllocateTSbuilder publishes epoch 8 in that gap, the delegated function seescurrent.epoch() > 0and returnsTxnNeedRetryWithDefChanged, even thoughCurrentValueexplicitly accepts a published cache of any epoch and no definition changed. I reproduced this deterministically by blocking the coldCurrentValueinGetColumns, publishing epoch 8, and then releasing it; the epoch-8 cache remained valid butCurrentValuereturned the retry error. Please make any-epoch a first-class mode inside the generation-fenced acquisition, including every recheck before and after construction, and add this interleaving as a regression. -
[P1] The service-wide lock can still wait on cache lifecycle work in the CREATE transaction-close path.
txnClosedcallshandleCreatesLockedwhile holdingservice.mu, and that helper still invokesprevious.retire(),tc.commit(), ortc.retire()under the lock.tableCache.retire/committake table and column locks; a column can hold its lock across allocator/store I/O. A deterministic rollback test with a blockedretireshowed an unrelatedReloadremaining blocked onservice.muuntil retirement was released, which is the same global lock-amplification failure mode fixed for Delete. Please mutate/remove the relevant map entries underservice.mu, collect commit/retire actions, and execute all lifecycle calls after unlocking. Cover committed and aborted CREATE, created-reset replacement, blocked commit/retire, and unrelated table operations.
Unhappy-path audit: Q1 store errors and caller cancellation are returned and generation counters are cleaned up; Q2 stale publication is generation-fenced, but loser side effects are not owned by a single build (finding 1); Q3 Delete/Reload/Close invalidation is substantially improved, but any-epoch reuse and CREATE close/retire still have uncovered interleavings (findings 2-3).
Validation: the unmodified pkg/incrservice suite and full -race suite pass. The three deterministic white-box interleavings above also ran under -race -count=20 after making the diagnostic store return independent column snapshots. git diff --check passes. The temporary diagnostic tests were removed from the worktree after verification.
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Reviewed head 68afae1 against origin/main and independently verified the change from first principles. Direction is correct: CurrentValue no longer builds caches under service.mu, committed deletes invalidate in-flight builders and always leave tombstones, and Delete retirement moved after unlock. I built libmo in this worktree and ran the full pkg/incrservice suite plus -race -count=5 on all new/affected tests: all pass.
The existing CHANGES_REQUESTED review (XuPeng-SH) targets exactly this head and the author has not yet responded; I independently confirmed all three P1 findings from the code, so I do not re-raise them: (1) cold CurrentValue bursts are not singleflighted — acquireCommittedTableCache misses delegate independently, generationBuilds is only a counter, and each newTableCache runs its own preAllocate -> asyncAllocate -> allocator worker, persistently advancing the AUTO_INCREMENT offset before N-1 loser caches are closed and their ranges abandoned; (2) the any-epoch contract lives only in the fast path — the delegated epoch-0 builder's rechecks (service.go:812-820 and 866-874) return TxnNeedRetryWithDefChanged when an epoch>0 cache publishes during the build; (3) txnClosed -> handleCreatesLocked still executes previous.retire()/tc.commit()/tc.retire() under service.mu, and tableCache.commit/retire take column locks that allocateLocked holds across allocator/store I/O — the same lock-amplification mode fixed for Delete.
Areas I examined with no new issues: generation bookkeeping closes on every builder exit path (start/finish/bump, error and cancel paths, Close resets the maps); the unconditional destroyed tombstone is safe given sqlStore.Delete semantics (succeeds on empty delete and on a vanished account; table IDs are globally unreused) and actually fixes a pre-existing mo_increment_columns GC gap when DROP runs on a CN that never cached the table; retiring deleted caches after unlock preserves acquire/release lifecycle with no double-retire hazard; Close/builders WaitGroup discipline is sound; removing CurrentValue's internal retry loop is an explicitly documented tradeoff tied to real definition changes. Test updates strictly improve coverage — TestCreateOnOtherService now genuinely exercises the other service (was ss[0], the creator, before).
|
Addressed all three P1 findings in e571eb8.
Added deterministic coverage for the 8-caller cold burst (one GetColumns and one Allocate), independent waiter cancellation, failed/canceled owner rebuilds, any-epoch publication, Delete/Reload/Close invalidation and wakeup, explicit-epoch owner+waiter supersession, committed/aborted/created-reset CREATE lifecycle paths, same-table fencing, unrelated-table progress, and Close waiting for pending commit. Validation passed:
Please re-review when convenient. |
matrix-meow
added
size/XL
4f480a2 to
3eca65b
Compare
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed exact head 3eca65b against merge base a5877b6. The three blockers from my previous review are closed: committed-cache construction is singleflight per table/generation, any-epoch CurrentValue semantics are preserved across every recheck and epoch supersession, and CREATE commit/retire work no longer holds service.mu while same-table users are fenced until commit completes.
I traced publication, invalidation, waiter cancellation, owner failure, Delete/Reload/Close, transaction commit/rollback, replacement retirement, and WaitGroup shutdown. Q1: each build and lifecycle action has one effective completion/cleanup owner. Q2: waiters are released by build completion, invalidation, context cancellation, or pending-commit completion without making unrelated tables wait. Q3: a cold burst shares one GetColumns/cache construction/allocation, avoiding duplicate persistent range consumption. I found no remaining correctness, liveness, resource, or material performance blocker.
Evidence: exact-head CI is complete and green; diff check passes; the 10 focused concurrency/lifecycle regressions were selected explicitly and passed locally for 5 consecutive runs with matching CGo artifacts. The author race evidence remains semantically valid because the final commit only replaces the internal sentinel construction for static-check compliance.
Merge Queue Status
This pull request spent 6 minutes 8 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #27597 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #27588
What this PR does / why we need it:
CurrentValuecold-cache construction held the service-wide mutex while reading committed columns and waiting for the first auto-increment range allocation. A slow fileservice/DiskCache operation could therefore block unrelated auto-increment work and DDL on the same CN, which amplified the storage stall observed in #27588.This change:
GetColumns, cache construction, and allocator/store I/O outsideservice.mu;CurrentValue, while returning generation invalidation and store errors to the caller without an internal retry loop;Regression coverage blocks both
GetColumnsand initial allocation to prove unrelated deletes can still acquire the service lock. It also covers Delete/Reload/Close invalidation, tombstone cleanup from another service, blocked cache retirement, and propagation of store retry errors without looping.Validation on
1312b142e4e9ee9bb4a4c172c804a789f74b9281rebased ontoorigin/main4343cb17f74aae790e7619a956c1d9bfc8997e84:GOWORK=off go list -mod=readonly ./pkg/incrservice: PASSpkg/incrservicefull test: PASSpkg/incrservicefull-racetest: PASS-race -count=100: PASSGOWORK=off go build -mod=readonly ./pkg/incrservice: PASSGOWORK=off go vet -mod=readonly ./pkg/incrservice: PASSgit diff --check origin/main...HEAD: PASS