Repository navigation
feat(storage): totally order table registrations by the manifest version - #686
Merged
azimafroozeh merged 1 commit intoSep 8, 2026
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
5 tasks done
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.
What & why
Table registrations in
__manifesthad no total order across native refs. #630 already fixed the bugs that came from that, with a rule in the merge path. This PR is the structural improvement behind it: it gives registrations a total order by construction, so the rule and the guards it needed go away. It closes no issue. RFC 0062 (docs/rfcs/0062-manifest-version-clock.md) ships in the same change; storage stamp v6 to v7.What it was. A registration is
(table_version, native_dataset_branch), and three sites ordered registrations bytable_versionalone. Lance numbers a branch's versions from its fork point, so that number orders only inside one native ref. The fold kept the greatest number per table, so an adopted registration with a lower number was invisible: the merge reportedfast_forwardand the target kept stale rows. The row key was that number, so an equal number on another ref replaced the stored row in place. The collision guard refused the equal-number cross-ref adopt withtable version N already exists ... with different state. #473 was the first of the family.How #630 fixed it.
adopt_requires_target_lineage: an adopt whose source number is at or below the target's goes through the target-lineage delta writer, so the registered number always grows on the target. Correct and shipped. What it leaves: the order is a rule every registering path must keep, the equal-number case pays a data write for a pointer move, and an equal number on another ref still passes the pin's version compare.How this one works. The
__manifestversion that publishes a registration is unique and increasing within a branch's lineage, because every graph branch is a Lance branch of__manifestand the fold compares one lineage. Each registration row now carries that version as the trailing segment of its row key (table_version:{stable:016x}:{inc:016x}:{manifest_version:020}), decoded on scan. The fold, the tombstone filter and the publisher's per-identity reductions select the greatest manifest version. The physical pointer,(published_dataset_version, native_dataset_branch), is unchanged. No column is added: a projected column costs one manifest read per fragment per change-feed page, whichtests/changes_cost.rscaps. The Arrow schema of__manifestis unchanged; RFC 0028 §4.5 is amended.Removed. The collision guard, the owner-branch handoff arm, the number-keyed
existing_versionsandexisting_tombstoneslookups, and #630's predicate with its routing. An adopt at a lower or equal number is a pointer switch, or a fork onto the target's own ref when the target has none.Added.
manifest_internalon read,OmniError::manifeston publish): two rows at one(identity, manifest version), a row stamped above the scanned version, a secondUpdateorTombstonefor one identity in one batch, a registration of a tombstoned identity, a tombstone of a sealed identity, a commit landing at a version other than the one stamped into its rows.TableVersionExpectationgainsnative_ref: NativeRefPin { Unchecked, Exact(Option<String>) };check_expected_table_versionsrefuses a winner at the pinned number on another native ref asReadSetChanged. This removes the pin gap fix(merge): preserve state when source table versions lag target #630 left open. Sidecar v9 and the 409 payload are unchanged.migrations.rs; a v6 graph is refused on open and rebuilt by export and load. The MemWAL v7 to v19 stamps never shipped, so v7 is reused.Backing issue / RFC
docs/rfcs/0062-manifest-version-clock.md, carried with its implementation because a stamp bump is a cutover;statusmoves toacceptedon approval. Amends RFC 0028 §4.5. The PR references bug: branch merge with a net-zero delta fails permanently with "table version already exists" #473 as a guard case only.Checklist
merge_adopt_equal_version_return_trip, guardissue_473_merge_net_zero_branch; fix(merge): preserve state when source table versions lag target #630's threemerge_adopt_*cases stay green without its predicate.db/manifest/tests.rs: later clock wins at an equal Lance number, lower Lance number on another ref wins the fold, batch duplicate refused, tombstoned and sealed identities refused, pin on one ref refused when the winner sits on another, warm fold and fresh reopen agree.crossversion_upgrade.rs:current_v7_refuses_and_rebuilds_genuine_v6_and_v6_refuses_v7underOMNIGRAPH_V6_BIN, the v0.9 journey rewritten to refuse and rebuild)docs/user/operations/upgrade.md,docs/releases/v0.11.0.md,docs/dev/versioning.md,recovery.md,writes.md,testing.md)__manifest's own version)Local verification
cargo test --workspace --no-fail-fast: 99 targets ok; two pre-existing failures, equally red on an untouched base:blob::tests::external_blob_file_policy_rejects_special_files(sandbox refusesUnixListener::bind) andomnigraph-gqt --lib tests::branch_list_shape_and_rows_are_blessed_like_a_read_step(stack overflow,dg/fix-rust-min-stack)cargo test -p omnigraph-engine --features failpoints --test failpoints: 152 passed in the workspace run withbranch_merge_phase_b_failure_recovered_on_non_main_targetred before its re-assertion below; that test passes alone after itcargo test -p omnigraph-gqt --test gq_logic_tests -- merge_adopt_: 4 passed;-- issue_473_merge_net_zero: passedcargo test -p omnigraph-engine --test changes_cost: passed (the column design failed it, 21 to 23 reads)cargo test -p omnigraph-cli --test crossversion_upgrade: compiles and skips locally (OMNIGRAPH_V09_BIN,OMNIGRAPH_V6_BINunset); CI'sTest WorkspaceandV5 ↔ V7 Format Fencerun themcargo clippy --workspace --all-targets,cargo clippy -p omnigraph-engine --features failpoints --all-targets,cargo fmt --all --check,python3 scripts/check-docs.py: cleanNotes for reviewers
omnigraph export --branch <b>per live branch,initandloadinto a new graph; branch topology, history and snapshots do not carry across. A converter is possible (_row_last_updated_at_versionholds every row's manifest version) and out of scope; RFC 0062's one unresolved question.branching.rs,merge_fast_forward.rsandfailpoints.rstests are re-asserted under the clock. A target owning its ref takes the source as a pointer switch at the source's version; a lazy target takes the existing fork arm. The lazy iteration ofbranch_merge_preserves_state_when_native_versions_differstops after its reads: a write on a ref forked from another branch's ref fails withNotFoundon the parent's_indices/(Index reads fail withNot found: .../tree/<parent>/_indices/...through a branch created from another branch lance-format/lance#7840), onmaintoday for a newer source as well; recorded as a known gap in the RFC's decision log. Inbranch_merge_phase_b_failure_recovered_on_non_main_targetthe two main-source cases are pointer switches with no phase-B effect: no sidecar, target untouched, the retry completes.blob_changed_only_adopt_uses_known_present_update's main-to-branch arm asserts a pointer switch: nothing staged, nothing probed, the branch reads main's external descriptor as stored.reclaim_orphaned_fork_and_refork. fix(merge): preserve state when source table versions lag target #630's delta route never orphaned a ref.Unchecked;heal_pending_recovery_sidecars_for_writereplays every pending sidecar before any writer runs, so nothing lands in between. Recording the read ref needs sidecar v10, a follow-up.graph_commitrows keep the manifest version in the overloadedtable_versioncolumn at this stamp; retiring that belongs to draft RFC 0058.