fix: order same-max-key nodes correctly and stop re-sorting pages on reload - #176
Merged
Merged
Conversation
…reload Retrospective-review follow-up to the duplicate-key reload fix; two real defects are corrected, and the reconstruction algorithm moves out of the proc macro into a unit-testable runtime helper. Defect 1: same-max-key nodes were ordered by node-id link --------------------------------------------------------- Several nodes can share one maximum key (a key's duplicates crossing leaf boundaries), and the persisted (key, link) node id alone cannot recover their relative order: links are row locations, uncorrelated with the lost discriminators. Ordering such nodes by link could place the mixed boundary page (the one also carrying smaller keys) after a duplicate-only page. That both stranded the smaller keys behind a bigger registered maximum and reset the per-key discriminator counter mid-run, so two nodes could end up with maxima that compare Equal under MultiPair's Ord (same key, same discriminator, different values) — and the outer node index is a map keyed by the maximum, so attaching the second node silently REPLACED the first. Nodes now order by (node id key, first entry key, node id link): in a valid snapshot at most one same-max-key node carries smaller keys and must come first; the rest are mutually order-free. Defect 2: pages were re-sorted, breaking positional CDC events -------------------------------------------------------------- get_node() already returns the page's logical (slot-resolved) entry order — the tree order the node had when persisted. That order is authoritative: CDC events are positional (InsertAt/RemoveAt carry an index into the node) and the on-disk page applies them through the same slot order; on removal of a node's maximum, the page promotes the LOGICAL predecessor to be the new node id. Re-sorting entries within a node (introduced together with the reload fix) desynced both: after a reload, a delete of a node maximum made the in-memory index and the table of contents disagree on the node's identity, and the persistence engine died on "page should be available in table of contents", hanging wait_for_ops. Reconstruction now preserves the get_node() order verbatim (for unique indexes too, restoring their original behavior) and only re-derives the cross-node discriminator ordering. Structure and tests ------------------- - New runtime helper reconstruct_multi_index_nodes (exported via the prelude) holds the whole algorithm plus structural validation of the persisted input: empty pages, a last entry that is not the node id, keys out of logical order, and backwards keys across the node sequence are reported with tracing::error and handled best-effort, keeping files damaged by earlier releases loadable. - Unit tests cover the review counterexample directly: a mixed boundary page ending in key K, a duplicate-only page of K, node-id links deliberately ordered opposite to the logical page order, and equal duplicate counts so maximum collisions are guaranteed under the old numbering; plus a multi-node straddle chain and damaged-input cases. Invariants asserted: exact entry preservation, per-node order kept verbatim, sorted nodes, non-overlapping ranges, maxima unique under Ord, input page order irrelevant. - The integration regression now uses an exact-id-set model (counts can hide one missing link plus one duplicated link), covers an unsized String duplicate index alongside the sized u64 one, asserts the straddling page topology it depends on by parsing the index file, and after the first reload runs a mutation battery (inserts into existing runs, spread deletes plus a whole key removed end to end, updates moving rows between keys) followed by a second reload — this battery is what exposed defect 2. A no-reload control keeps the live CDC path and the reload path distinguishable. A single-key all-duplicates stress test rounds it out. Suite: 319 passed / 0 failed / 4 ignored, 5 consecutive runs; fmt and clippy clean. Cross-version harness: files written by earlier releases still read back complete (8572/8572, 10000/10000).
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
Corrective PR for the retrospective review of #175. The reviewer's P1 was real, and the demanded post-reload mutation battery exposed a second defect on top of it. Both are fixed, and the algorithm now lives in a unit-testable runtime helper (
reconstruct_multi_index_nodes) with the review counterexample as a direct unit test.Defect 1 (the review's counterexample): same-max-key node ordering
The persisted
(key, link)node id cannot recover the relative order of several nodes sharing one maximum key — links are row locations, uncorrelated with the lost discriminators. Ordering by link could place the mixed boundary page after a duplicate-only page, stranding the smaller keys and resetting the per-key discriminator counter mid-run so two node maxima comparedEqualunderMultiPair::cmp(same key, same discriminator, different values). The outer node index is a map keyed by the maximum, so the second attach silently replaced the first node. Fixed by ordering nodes by(node id key, first entry key, node id link)— in a valid snapshot at most one same-max-key node carries smaller keys and must come first.Defect 2 (found by the mutation battery): re-sorting broke positional CDC events
get_node()already returns the page's logical slot-resolved order — the authoritative tree order. CDC events are positional, the on-disk page applies them through the same slot order, and on removal of a node's maximum the page promotes the logical predecessor as the new node id. The reload fix's within-page re-sort desynced this: after reload + deletes, the in-memory index and the TOC disagreed on a node's identity and the engine died on "page should be available in table of contents", hangingwait_for_ops. Reconstruction now preservesget_node()order verbatim and only re-derives cross-node discriminator ordering. (This also corrects the earlier "pages are arrival-ordered" narrative: the cells are, the slot-resolved view is not.)Review checklist coverage
tracing::error!with index name and node id, best-effort continue (hard-failing would brick files damaged by earlier releases;from_persisted's infallible signature is noted as the API follow-up).Stringduplicate index asserted with exact id sets through both reloads, and the topology assertion parses the index file to prove duplicates actually straddle pages.Verification