fix: lossy reconstruction of duplicate-key secondary indexes on reload - #175
Merged
Conversation
…ex persistence
Problem
-------
A plain bulk insert of 10_000 rows spread over 97 distinct values of a
non-unique secondary index (~103 duplicates per key) is fully visible
through select_by_* while the table is in memory, but after wait_for_ops
and a reopen via load() roughly 12% of the entries are no longer
reachable through the index. select_all still returns every row, so the
data pages are intact; only the persisted index is incomplete.
The loss is baked into the index file at write time: re-reading the same
files gives the same wrong counts, while separate write runs lose
different amounts. An extra wait_for_ops plus a sleep before shutdown
does not help, so this is not the known wait_for_ops race. Unique-valued
secondary indexes survive the identical round trip intact.
The existing assertions that would have caught this are commented out in
tests/persistence/sync/string_secondary_index.rs (lines 449-452).
Reproduction
------------
The test is #[ignore]d so CI stays green until the fix lands. Run with:
cargo test --test mod test_duplicate_key_secondary_index_survives_reload -- --ignored
Current failure on master:
secondary index lost 968 of 10000 entries across persist+reload
pathscale
force-pushed
the
tests/duplicate-key-index-reload-repro
branch
from
July 27, 2026 11:27
975c4b7 to
cd8d8c6
Compare
Problem ------- Index pages store entries in event-arrival order: only bootstrap-written pages happen to be sorted with the node maximum last, while pages that were incrementally updated through CDC events (any bulk load) are arbitrary. from_persisted assumed "last entry == node maximum" for non-unique indexes: it popped the last entry of every page and re-appended it with discriminator u64::MAX - 1 as the node id. For CDC-updated pages that promoted an arbitrary entry to node maximum, so the reconstructed in-memory node index registered wrong maxima and every entry sorting above one became unreachable through select_by_* (~12% of entries in the regression workload; ~62% under 0.9.0-beta), even though select_all still returned every row. The wrong in-memory maximum also desynced reloaded tables from the on-disk table of contents, which addresses nodes by their maximum: CDC events emitted after a reload could reference node ids the TOC never had. Fix --- Reconstruction now: - sorts every page's entries (a no-op for bootstrap-written pages), - forces the page's persisted node_id entry to sort last within its key, so the reconstructed node maximum is exactly the entry the TOC knows the node by, - attaches nodes in ascending order of their node id, and - hands out discriminators that keep growing across node boundaries within one key, so a key's duplicates straddling nodes keep the B-tree ordering invariant (every entry <= its node's registered maximum). Discriminator 0 stays reserved for the range infimum and u64::MAX for the supremum. Unique indexes get the page sort as well: their reconstruction assumed sorted pages the same way, it just never had duplicate keys to lose. The reload fix is retroactive for 0.9.0-written files: the entries were always all present in the files (verified by parsing pages directly), only reconstruction lost them, so existing tables read back complete without a rewrite. Files written by 0.9.0-beta had additional write-side damage and recover only partially. The repro test is un-ignored, asserts exact per-key counts after reload, and passes 5/5 full-suite runs (317 passed / 4 ignored).
pathscale
changed the title
pathscale
pushed a commit
to pathscale/api.support.cafe
that referenced
this pull request
Jul 27, 2026
Picks up the duplicate-key secondary-index reload fix (pathscale/WorkTable#175): reloaded tables now reconstruct non-unique indexes completely instead of losing entries whose key straddles index-node boundaries, and reloads stay aligned with the on-disk table of contents so post-reload persistence cannot address missing nodes. Also returns to the worktable_codegen crate name (worktable_macros was a one-release workaround for 0.9.0). Verified locally: default and s3-sync feature builds, migration crate build, fresh start plus reload-from-disk run with no errors.
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
1 participant
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
Root-causes and fixes the ~12% loss of duplicate-key secondary-index entries across persist+reload, and turns the repro into a permanent regression test (now un-ignored, asserting exact per-key counts).
Root cause
Index pages store entries in event-arrival order — only bootstrap-written pages happen to be sorted with the node maximum last.
from_persistedassumed "last entry == node maximum": it popped the last entry of every page and re-appended it as the node id with discriminatoru64::MAX - 1. For CDC-updated pages (any bulk load) that promoted an arbitrary entry to node maximum, so:select_by_*(select_allstill returned every row — the files always contained all entries, verified by parsing pages directly);Fix
Reconstruction now sorts every page's entries, forces the page's persisted
node_identry to sort last (so the in-memory node maximum is exactly the entry the TOC knows the node by), attaches nodes in ascending node-id order, and assigns discriminators that keep growing across node boundaries within one key — restoring the B-tree ordering invariant when one key's duplicates straddle nodes. Unique indexes get the page sort too (same latent assumption, no duplicates to lose).Verification
secondary index lost 968 of 10000 entries) and passes with the fix, asserting exact per-key counts after reload.string_re_readreload tests (TOC desync surfaced while iterating on the fix) pass — the node-id alignment is what makes post-reload writes safe.