fix: hold meta-page lock across reinit and writes in MetaPage::store - #270
Draft
pavanmanishd wants to merge 1 commit into
Draft
fix: hold meta-page lock across reinit and writes in MetaPage::store#270pavanmanishd wants to merge 1 commit into
pavanmanishd wants to merge 1 commit into
Conversation
Parallel index-build workers all call MetaPage::store(false) from finalize_index_build, which released the exclusive buffer lock between reinit and each subsequent write. Concurrent workers could interleave reinit/write and trigger the "offset N != 1" assertion failure on block 0. Restructure store() to hold one WritablePage across reinit, header write, and meta write, committing once at the end. Extract write_chain_item_to_page helper for the single-item, caller-holds-lock case so the chain-item serialization stays in util/chain.rs. Remove the now-unused ChainTapeWriter::reinit. Fixes timescale#264
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Parallel DiskANN index builds can fail with
assertion 'left == right' failed(see #264). Every parallel worker callsMetaPage::store(false)fromfinalize_index_build(build.rs:940), andstore()released the exclusive buffer lock betweenreinitand each subsequentwrite. Concurrent workers could interleave the operations and produce the observed offset mismatch.This PR restructures
MetaPage::storeto hold oneWritablePageacross reinit and both writes, committing once at the end. Concurrent callers now serialize on the Postgres buffer lock.Note for reviewers: the root cause is slightly different from what's described in #264 — the
MetaV1branch offetch()is not the trigger. Freshly built indexes usePageType::Meta, andfetch()on that branch is read-only (Self::load). The race I'm seeing is inMetaPage::storeitself, exercised fromfinalize_index_buildin each parallel worker.Changes
pgvectorscale/src/access_method/meta_page.rs—store()now acquires a singleWritablePage(vianewforfirst_time=true, ormodify+ in-placereinitforfirst_time=false), writes header + meta, and commits once.pgvectorscale/src/util/chain.rs— newwrite_chain_item_to_page(page, data)helper for the single-item, caller-holds-lock case (keeps theChainItemHeaderserialization format in one module). Removed the now-unusedChainTapeWriter::reinit.Net diff: +33 / −34 lines across 2 files.
Test plan
cargo pgrx install --release --features pg18builds cleanly; no new clippy warnings on the changed files.mainwith a 50 mssleepinserted betweenreinitand the firstwriteinstore(): 4 of 5 iterations fail with the exact assertion from [Bug]: <Title>Parallel_Build_Assertion_Failure #264.sleepinserted inside the single lock window to stress serialization: 5 of 5 iterations pass.cargo pgrx test pg{17,18}— to be confirmed by CI (local run blocked by a pgrx--sudoinstall quirk on macOS, unrelated to this change).Repro script preserved for convenience:
Notes
I intentionally did not add an in-tree regression test because reproducing the bug deterministically requires injecting a sleep inside
store(). Happy to add a#[cfg(test)]-gated race-widener test on request.Fixes #264