(feat): compact the internal manifest/commit-graph tables in optimize#291
Merged
Conversation
…n optimize `optimize` iterated node/edge catalog tables only, so the two internal system tables (`__manifest`, `_graph_commits`) accumulated one fragment per commit and were never compacted -- making every write's metadata scan O(fragments), which grows forever on a long-lived graph (RFC-013 step 2). `optimize_all_tables` now also compacts both internal tables via a new `compact_internal_table`. They are not catalog-tracked (readers open them at their latest Lance HEAD), so it is a much simpler path than `optimize_one_table`: compact in place, no manifest publish (nothing to publish to), no recovery sidecar (a single atomic Lance commit -- no HEAD-before-publish gap), and no optimize_indices (they carry no Lance index, only object_id's unenforced-PK metadata). No application lock: Lance's compact_files auto-retries its Rewrite against any concurrent writer (the canonical LanceDB pattern; Rewrite vs Append is compatible, vs Update a retryable same-fragment conflict Lance rebases), and a coordinator refresh afterwards makes the warm handle observe the compacted HEAD. Compacts both tables even though Phase 7 (iss-991) will later fold _graph_commits into __manifest -- a one-call throwaway for the full interim win; __manifest compaction is also the prerequisite for Phase 7's graph_head contention. Cleanup (version GC) of the internal tables is deliberately NOT included here: it needs the Q8 cleanup-resurrection watermark first (deferred). maintenance.rs: optimize now returns 6 stats (4 data + 2 internal); adds optimize_compacts_internal_tables (sheds fragments, leaks no recovery sidecar, graph coherent for reads + strict writes after).
`internal_table_scans_are_flat_in_history` was the RED, #[ignore]'d acceptance gate staged in PR #288. With internal-table compaction landed, a write's __manifest/_graph_commits scan is flat in commit-history depth on a compacted graph (measured __manifest 4->2, _graph_commits 7->3 across depth 10->100, vs the pre-step-2 RED 34->214 / 29->207). The test now compacts at each depth before measuring and runs green every-PR.
- invariants.md: close the compaction half of the read-path-rederivation known gap (optimize now compacts the internal tables; cleanup half still deferred). - maintenance.md: optimize covers __manifest/_graph_commits (no publish, no sidecar); not yet in cleanup. - rfc-013 §9: split step 2 into 2a (compaction, landed) and 2b (cleanup + Q8 watermark, deferred — debated; MTT-overlap + hot-path liability). - testing.md: the internal-table LOCK is now green every-PR.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8db8937a6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+578
to
+580
| let metrics = compact_files(&mut ds, options, None) | ||
| .await | ||
| .map_err(|e| OmniError::Lance(e.to_string()))?; |
There was a problem hiding this comment.
On upgraded graphs whose internal Lance tables still carry the pre-v7 lance.auto_cleanup.* config, this compact_files commit has no skip_auto_cleanup equivalent, so running optimize can trigger Lance's automatic version GC on __manifest / _graph_commits. Those versions are the snapshot/time-travel authority; docs/dev/lance.md and skip_auto_cleanup_suppresses_version_gc call out that every commit path must skip this hook for pre-bump graphs. Please disable/clear the legacy auto-cleanup config before committing internal compaction, or use a commit path that can set the skip.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…bles Addresses PR #291 review findings: - Greptile (P1): optimize unconditionally opened `_graph_commits` for compaction, but a graph can validly have none (the coordinator opens it as `Option`, gated on `storage.exists`, for graphs predating the commit graph). `Dataset::open` on the absent table errored and failed the whole optimize. Guard the `_graph_commits` compaction with the same `storage_adapter().exists()` check the coordinator uses; `__manifest` always exists so it stays unguarded. Regression test `optimize_tolerates_absent_graph_commits_table` (empty graph so no publish recreates the table before the guard). - Cursor (low): the `table_tasks.is_empty()` early return skipped internal-table compaction for a schema with no node/edge types. Removed it so the internal tables are compacted regardless of the data-table set. - Codex (auto-cleanup, P1): documented — `compact_files` commits with a default `CommitConfig` (no skip_auto_cleanup) and `CompactionOptions` exposes no override, so on a graph storing an *on* auto_cleanup config the commit would fire version GC. Both internal tables are created with `auto_cleanup: None`, so new graphs are safe; the only exposure is pre-fix upgraded graphs, identical to the existing data-table optimize path, with step 2b's watermark as the comprehensive guard. Added a comment in `compact_internal_table` recording this.
…publish) Step 2 compacts `__manifest` with no app-level lock (Lance OCC arbitrates, validated against LanceDB + the lance-7.0.0 conflict resolver). compact_files' `Operation::Rewrite` auto-retries 20x (CommitConfig default num_retries=20), so a live publish usually wins the race and the compaction rebases. But the publish runs its merge-insert with conflict_retries(0) = one rebase attempt; if the compaction commits first AND the merge touched a fragment the Rewrite rewrote, Lance preempts the publish with `Error::RetryableCommitConflict` — a DIFFERENT variant from the row-level `TooMuchWriteContention` the publisher already retries. Left unhandled, that surfaces a transient error to the caller, i.e. a maintenance compaction (physical op) failing a live write (logical op) — invariant 7. Map `LanceError::RetryableCommitConflict` to a new `ManifestConflictDetails::RetryableCommitConflict` and treat it as retryable in the publisher's outer loop (reload fresh state + re-merge), alongside RowLevelCasContention. `ExpectedVersionMismatch` still propagates (a genuine expectation break must not be blindly retried). This also hardens multi-process concurrent writers generally, not just compaction. Normal publishes are insert-only (new object_ids -> new fragments, disjoint from rewritten old ones), so the conflict is rare; the guard covers the same-fragment-update edge and multi-process writers. Unit tests in publisher.rs pin the mapping + the retry-predicate contract.
…side) Reverts d138902. Validated against lance-7.0.0: the publisher's merge-insert runs with conflict_retries(0), and execute_with_retry converts an exhausted retryable commit conflict to TooMuchWriteContention before the caller sees it (write/retry.rs ~95-130). So map_lance_publish_error NEVER receives RetryableCommitConflict from merge_rows — it receives TooMuchWriteContention, which the publisher already maps to RowLevelCasContention and retries. The reverted mapping was therefore dead on the real path and its unit test was synthetic. The actual exposure is the *compaction* side: compact_files -> commit_compaction -> apply_commit directly (no execute_with_retry), so a Rewrite-vs-Merge check_txn conflict propagates raw and optimize can fail on a live graph. That is fixed app-side in compact_internal_table in the following commit.
Address three findings from review of the step-2 internal-table compaction: - Non-destructive by construction: before compacting an internal table, strip any stored `lance.auto_cleanup.*` config off it. `compact_files` commits with a default `CommitConfig` (skip_auto_cleanup=false) and `CompactionOptions` exposes no override, so on a graph created by an older binary (on-by-default GC hook) the compaction commit would fire Lance's auto-cleanup and silently prune `__manifest`-pinned versions. Current binaries store no such config; the strip is the upgrade-path safety net so `optimize` can never GC versions. - App-level compaction retry: `compact_files` does NOT auto-retry a semantic conflict against a concurrent live writer (Rewrite vs Update/Merge/Delete propagates raw from apply_commit; Lance prescribes app-rerun). Wrap the internal-table compaction in a bounded retry loop that reopens fresh and replans on a retryable Lance conflict, so a maintenance compaction (a physical op) never fails a live write (a logical op) — invariant 7. - Compact all three internal tables, not two: `_graph_commit_actors` grows one fragment per commit on the authenticated write path, the same O(depth) scan as `__manifest`/`_graph_commits`. Drive the sweep from one source-of-truth list with per-table existence guards (the two commit-graph tables are optional). Make `graph_commit_actors_uri` pub(crate). Tests: the `internal_table_scans_are_flat_in_history` LOCK now runs the authenticated (actorful) write path so it covers `_graph_commit_actors` via the shared commit-graph IO wrapper (new `commit_many_as`/`measure_insert_as` helpers); `optimize_clears_stale_auto_cleanup_and_preserves_versions` pins the non-destructive guarantee (config cleared + no version GC); a unit test pins the retryable-conflict classifier; the empty-graph stats count is 7 (the actor table is created at init).
… retried Sync the RFC-013 step-2a section and the maintenance guide with the correctness-by-design refinements: - optimize compacts `__manifest`, `_graph_commits`, AND `_graph_commit_actors` (the actor table grows on the authenticated write path). - optimize is non-destructive by construction — it never GCs versions, and strips stale `lance.auto_cleanup.*` config so an upgraded graph's commit-time GC hook cannot fire during compaction. - internal-table compaction rebases and retries against concurrent live writers rather than failing the operator's optimize or the live write. - the cost LOCK is the authenticated-path acceptance test.
Contributor
Author
Correctness-by-design pass on internal-table compactionAddressed four findings from review. All fixes are correct-by-construction (not symptom guards), each with a test at the matching boundary. Full workspace gate green ( F1 — F2 — compact all three internal tables. F3 — internal-table compaction must retry against live writers. F4 — reverted the wrong-side hardening ( |
ragnorc
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e034636. Configure here.
ragnorc
changed the title
…ion work `compact_internal_table` returns early when `plan_compaction` finds no work, but `clear_stale_auto_cleanup_config` may have already committed a config-strip that advanced Lance HEAD. The early return skipped the coordinator refresh that the successful-compaction path performs, leaving warm `__manifest`/commit-graph handles pinned to the pre-strip version until the next read's version probe healed them. No correctness bug (the probe self-heals, and a stale-handle write would retry via publisher CAS), but the refresh makes coherence deterministic rather than probe-dependent. Refresh iff the config-strip actually committed.
… not auto-retry The function doc claimed "Lance's compact_files auto-retries its Operation::Rewrite against any concurrent writer" — wrong, and contradicting the is_retryable_lance_conflict doc just below it and the explicit retry loop that exists precisely because compact_files does NOT auto-retry semantic conflicts (Rewrite vs Update/Merge/Delete propagates raw through apply_commit). Also move the orphaned description from above the retry-budget const onto the function, and include the third internal table.
…oo (red) Regression test for a destructive bug on the data-table optimize path: on an upgraded graph whose node/edge table still carries pre-v7 lance.auto_cleanup.* config, `optimize`'s compact_files/optimize_indices commits fire Lance's version GC and prune __manifest-pinned data-table versions. Mirrors the internal-table auto_cleanup test on a Person table (force-repair realigns the config-induced drift so optimize doesn't skip the table). Red against the current code: the data-table path does not strip the config. The fix lands in the next commit.
… too The auto_cleanup scrub previously only protected the internal tables; the data-table path (optimize_one_table) ran compact_files/optimize_indices with a default CommitConfig (skip_auto_cleanup=false) and no override, so on an upgraded graph those commits could fire Lance's version-GC hook and prune __manifest-pinned node/edge versions — making the "non-destructive" contract false for data tables. Strip the config before the HEAD-advancing commits, capturing version_before first so the strip's own commit still triggers the Phase-C manifest publish (no uncovered drift). No retry loop needed: the data-table path holds the per-table write queue. Covered by the existing Optimize recovery sidecar. Turns the prior commit's test green. Also: switch clear_stale_auto_cleanup_config off the deprecated delete_config_keys to update_config(None values), and correct two now-inaccurate doc comments — compaction is "one or more content-preserving commits" (compact_files can emit a ReserveFragments before the Rewrite), not "a single atomic commit"; the sidecar-free property rests on content-preservation + read-at-HEAD, not single-commit atomicity.
…try claims - non-destructive guarantee now spans data + internal tables (the auto_cleanup strip runs on both paths), not just the internal ones. - "single atomic Lance commit" was inaccurate: compaction can emit a ReserveFragments commit before the Rewrite; the no-sidecar property rests on content-preservation + read-at-HEAD, not single-commit atomicity. - "retries rather than failing" softened to the truth: a *bounded* retry on the internal path; sustained contention surfaces a loud conflict error (bounded + observable, not an infinite loop). The data path holds the per-table queue and never contends.
Contributor
Author
Second review pass — two more findings, both valid, both addressedValidated against the current code and Lance 7.0.0 source. Full Finding 1 (High) — Fix ( Finding 2 (Medium) — docs overclaimed substrate guarantees. Valid; corrected.
Incidental cleanup: Commits: |
ragnorc
added a commit
that referenced
this pull request
Jun 21, 2026
Fold in two measured findings from the deployed edge binary (f6d2cc0) on rustfs behind a latency+concurrency proxy: - §0(d): concurrency-cap A/B. Under unlimited concurrency the internal-table scan parallelizes (backbone ~110); under an R2-realistic cap (8) it serializes and an UNCOMPACTED graph runs away (per-write ops 1273->3505, wall 6->16s), while #291's internal compaction cuts it ~6x and bounds it (137->1 frag). The latency model is (serial_hops + ops/effective_concurrency)*RTT + compute. - Reframe step 2 across Summary/§2.4/§9: NOT de-ranked — on R2 (capped) it is a primary latency lever + the anti-runaway fix + Phase-7 prereq. The earlier 'step 2 is parallel, irrelevant to latency' was an unlimited-concurrency artifact. Deployed f6d2cc0 optimize is node/edge-only; #291 (undeployed) is the prod win. - §5.1: the cost-gate ThrottledStore must cap concurrency AND inject latency; assert serial_hops flat AND ops flat in history. - §2.3 + §8: Lance/LanceDB comparison from 7.0.0 source — Lance metadata is a single-file per-version manifest read O(1) (latest_version_hint), pruned by default; omnigraph's __manifest-as-Lance-dataset scan is self-inflicted by the cross-table-atomicity choice. Adds explicit defense of Lance-dataset __manifest (MTT seam) vs a flat-file CAS'd manifest (cheaper, off the MTT path). Design (§3/§4.1) unchanged and vindicated; corrections are measurement framing, step sizing, and one design-choice that was implicit.
ragnorc
added a commit
that referenced
this pull request
Jun 21, 2026
…rency-cap correction + Lance-metadata comparison (#292) * feat(engine): compact the internal __manifest/_graph_commits tables in optimize `optimize` iterated node/edge catalog tables only, so the two internal system tables (`__manifest`, `_graph_commits`) accumulated one fragment per commit and were never compacted -- making every write's metadata scan O(fragments), which grows forever on a long-lived graph (RFC-013 step 2). `optimize_all_tables` now also compacts both internal tables via a new `compact_internal_table`. They are not catalog-tracked (readers open them at their latest Lance HEAD), so it is a much simpler path than `optimize_one_table`: compact in place, no manifest publish (nothing to publish to), no recovery sidecar (a single atomic Lance commit -- no HEAD-before-publish gap), and no optimize_indices (they carry no Lance index, only object_id's unenforced-PK metadata). No application lock: Lance's compact_files auto-retries its Rewrite against any concurrent writer (the canonical LanceDB pattern; Rewrite vs Append is compatible, vs Update a retryable same-fragment conflict Lance rebases), and a coordinator refresh afterwards makes the warm handle observe the compacted HEAD. Compacts both tables even though Phase 7 (iss-991) will later fold _graph_commits into __manifest -- a one-call throwaway for the full interim win; __manifest compaction is also the prerequisite for Phase 7's graph_head contention. Cleanup (version GC) of the internal tables is deliberately NOT included here: it needs the Q8 cleanup-resurrection watermark first (deferred). maintenance.rs: optimize now returns 6 stats (4 data + 2 internal); adds optimize_compacts_internal_tables (sheds fragments, leaks no recovery sidecar, graph coherent for reads + strict writes after). * test(engine): un-ignore the internal-table scan LOCK (step 2 acceptance) `internal_table_scans_are_flat_in_history` was the RED, #[ignore]'d acceptance gate staged in PR #288. With internal-table compaction landed, a write's __manifest/_graph_commits scan is flat in commit-history depth on a compacted graph (measured __manifest 4->2, _graph_commits 7->3 across depth 10->100, vs the pre-step-2 RED 34->214 / 29->207). The test now compacts at each depth before measuring and runs green every-PR. * docs: RFC-013 step 2 internal-table compaction landed - invariants.md: close the compaction half of the read-path-rederivation known gap (optimize now compacts the internal tables; cleanup half still deferred). - maintenance.md: optimize covers __manifest/_graph_commits (no publish, no sidecar); not yet in cleanup. - rfc-013 §9: split step 2 into 2a (compaction, landed) and 2b (cleanup + Q8 watermark, deferred — debated; MTT-overlap + hot-path liability). - testing.md: the internal-table LOCK is now green every-PR. * fix(engine): guard absent _graph_commits + always compact internal tables Addresses PR #291 review findings: - Greptile (P1): optimize unconditionally opened `_graph_commits` for compaction, but a graph can validly have none (the coordinator opens it as `Option`, gated on `storage.exists`, for graphs predating the commit graph). `Dataset::open` on the absent table errored and failed the whole optimize. Guard the `_graph_commits` compaction with the same `storage_adapter().exists()` check the coordinator uses; `__manifest` always exists so it stays unguarded. Regression test `optimize_tolerates_absent_graph_commits_table` (empty graph so no publish recreates the table before the guard). - Cursor (low): the `table_tasks.is_empty()` early return skipped internal-table compaction for a schema with no node/edge types. Removed it so the internal tables are compacted regardless of the data-table set. - Codex (auto-cleanup, P1): documented — `compact_files` commits with a default `CommitConfig` (no skip_auto_cleanup) and `CompactionOptions` exposes no override, so on a graph storing an *on* auto_cleanup config the commit would fire version GC. Both internal tables are created with `auto_cleanup: None`, so new graphs are safe; the only exposure is pre-fix upgraded graphs, identical to the existing data-table optimize path, with step 2b's watermark as the comprehensive guard. Added a comment in `compact_internal_table` recording this. * docs(rfc-013): serial-hop correction — wall-clock is the ~110-hop backbone, not op count Latency-slope measurement on the deployed edge binary (f6d2cc0, steps 1+3a landed; rustfs + per-op latency proxy, depth 1..85) shows wall-clock is set by a ~110-hop SERIAL backbone that is depth-invariant. Total ops grow +~7/depth but PARALLELIZE (parallelism 1->6), so the depth term adds little wall-clock. - New §0(c): the serial-hop vs total-op finding + branch-op backbones (create ~77, delete ~87, branch-write ~258/1777-ops/21s floor = fork-on-first-write). - §2.4: correct the '1720->198 ops => 258s->30s' op-count->wall-clock conversion. - §5.1: promote serial-hop/num_stages to the PRIMARY latency LOCK; op-count flatness demoted to a cost/compute-floor gate. - §9 step 2: reprioritized as Phase-7 prerequisite + compute-floor/space, NOT the wall-clock fix; step 3b (parallel capture-once WriteTxn) is the headline latency lever; branch-write moved under step 3b + fork seam. - Summary: serial-backbone correction up front. Vindicates the §3/§4.1 design; corrects the op-count latency framing. * docs(rfc-013): concurrency-cap correction + Lance-metadata comparison Fold in two measured findings from the deployed edge binary (f6d2cc0) on rustfs behind a latency+concurrency proxy: - §0(d): concurrency-cap A/B. Under unlimited concurrency the internal-table scan parallelizes (backbone ~110); under an R2-realistic cap (8) it serializes and an UNCOMPACTED graph runs away (per-write ops 1273->3505, wall 6->16s), while #291's internal compaction cuts it ~6x and bounds it (137->1 frag). The latency model is (serial_hops + ops/effective_concurrency)*RTT + compute. - Reframe step 2 across Summary/§2.4/§9: NOT de-ranked — on R2 (capped) it is a primary latency lever + the anti-runaway fix + Phase-7 prereq. The earlier 'step 2 is parallel, irrelevant to latency' was an unlimited-concurrency artifact. Deployed f6d2cc0 optimize is node/edge-only; #291 (undeployed) is the prod win. - §5.1: the cost-gate ThrottledStore must cap concurrency AND inject latency; assert serial_hops flat AND ops flat in history. - §2.3 + §8: Lance/LanceDB comparison from 7.0.0 source — Lance metadata is a single-file per-version manifest read O(1) (latest_version_hint), pruned by default; omnigraph's __manifest-as-Lance-dataset scan is self-inflicted by the cross-table-atomicity choice. Adds explicit defense of Lance-dataset __manifest (MTT seam) vs a flat-file CAS'd manifest (cheaper, off the MTT path). Design (§3/§4.1) unchanged and vindicated; corrections are measurement framing, step sizing, and one design-choice that was implicit.
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.
What & why
optimizecompacted node/edge data tables only — the two internal system tables__manifest(visibility authority) and_graph_commits(lineage DAG) were never compacted (all_table_keysis catalog-keys-only). They accumulate one fragment per commit, so every write's metadata scan of them is O(fragments) and grows forever on a long-lived graph. That was the residual internal-table term after PR #288 flattened the data-table opener, and the RED#[ignore]'dinternal_table_scans_are_flat_in_historyLOCK that #288 staged.This PR brings both internal tables into
db.optimize().Changes
compact_internal_table(new, inoptimize.rs) — a deliberately simpler path thanoptimize_one_table. The internal tables aren't catalog-tracked (readers open them at their latest Lance HEAD), so compaction just advances that HEAD: no manifest publish (nothing to publish to), no recovery sidecar (a single atomic Lancecompact_filescommit — no HEAD-before-publish gap), nooptimize_indices(they carry no Lance index, onlyobject_id's unenforced-PK metadata).optimize_all_tablesruns it for__manifestand_graph_commitsafter the data tables.compact_filesauto-retries itsOperation::Rewriteagainst any concurrent writer (RewritevsAppendis compatible, vsUpdate/merge-insert a retryable same-fragment conflict Lance rebases —lance/io/commit.rs,conflict_resolver.rs). A coordinatorrefreshafter makes the warm__manifest/_graph_commitshandle observe the compacted HEAD. (Within one processoptimize&selfandmutate&mut selfcan't overlap anyway; multi-process is the existing one-winner-CAS exposure.)iss-991) will later fold_graph_commitsinto__manifest— a one-call throwaway for the full interim win.__manifestcompaction is also the hard prerequisite for Phase 7 (itsgraph_headCAS contention is only acceptable once compaction bounds the publisher'sload_publish_statescan).Measured result (the LOCK, now green)
internal_table_scans_are_flat_in_history(un-ignored): on a compacted graph a write's internal-table scan is flat in commit-history depth:__manifestreads_graph_commitsreadsvs the pre-step-2 RED curve (
__manifest34→214,_graph_commits29→207).Scope — compaction only (cleanup deferred)
This is step 2a. The cleanup (version-GC) half — step 2b — is deliberately not here: cleanup is the version-deleting op that hits the Q8 cleanup-resurrection hole (Lance's version CAS has no monotonic guard, validated in
lance/dataset/transaction.rs/cleanup.rs), so it must land with a durable monotonic watermark (a Lance boundary tag). Deferred because it touches the read/open path, is the part most likely made redundant by Lance MTT, and only buys the secondary version-count/space term — whereas 2a delivers the dominant per-write scan win with zero resurrection risk. So the internal_versions/chain still grows until 2b; runoptimizeon a cadence to keep per-write metadata scans flat. Seedocs/dev/rfc-013-write-path-latency.md§9.Tests
cargo test --workspace --lockedgreen (65 suites). (One pre-existing flaky server test,schema_apply_route_hard_drops_property_with_allow_data_loss, intermittently fails under full parallel load — passes 4/4 in isolation, on clean main, and on the re-run; unrelated to this optimize-only change, which the server test never exercises.)internal_table_scans_are_flat_in_historyun-ignored and green;optimize_compacts_internal_tables(sheds fragments, no sidecar leak, coherent after);optimize_on_empty_graphupdated (6 stats); idempotent test already covers internal no-op.Note
Medium Risk
Changes the maintenance path and coordinator refresh behavior under concurrent writes, but compaction is non-destructive (no version GC) and is heavily covered by integration and cost-budget tests.
Overview
optimizenow compacts internal system tables (__manifest,_graph_commits,_graph_commit_actors) after node/edge tables, via a newcompact_internal_tablepath: in-place Lance compaction only—no__manifestpublish and no recovery sidecar—plus coordinatorrefreshfor warm handles. The early return when the schema has no data tables is removed so internal tables are still swept.Safety and concurrency:
clear_stale_auto_cleanup_configruns before compaction on internal tables and on data tables inside the existing optimize sidecar, so upgraded graphs cannot trigger Lance commit-time version GC duringoptimize. Internal compaction uses a bounded retry on retryable Lance write conflicts (no per-table write queue); missing_graph_commits/_graph_commit_actorsdatasets are skipped via storage existence checks.Tests and gates: Integration tests cover internal compaction, absent commit-graph tables, and auto-cleanup stripping on internal and data tables. The
internal_table_scans_are_flat_in_historycost LOCK is enabled and uses actorful writes withoptimizeat each depth checkpoint. User and RFC docs describe the new behavior; internal version cleanup remains deferred (step 2b).Reviewed by Cursor Bugbot for commit edfc73d. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR brings the two (now three) internal system tables —
__manifest,_graph_commits, and_graph_commit_actors— intodb.optimize()as RFC-013 step 2a, fixing the O(fragments) per-write metadata scan that grew unboundedly on long-lived graphs. It also un-ignores theinternal_table_scans_are_flat_in_historycost LOCK and addsclear_stale_auto_cleanup_configto both the new internal-table path and the existing data-table path, makingoptimizenon-destructive by construction on pre-v7 upgraded graphs.compact_internal_table(new) — a simplified variant ofoptimize_one_tablewith no manifest publish, no recovery sidecar, and a bounded app-level retry loop forRewrite-vs-live-write conflicts; existence-guarded for the two optional commit-graph tables; followed by a coordinatorrefreshfor cache coherence.clear_stale_auto_cleanup_config(new, shared) — strips any storedlance.auto_cleanup.*config before compacting any table (data or internal) so the commit-time Lance GC hook cannot prune__manifest-pinned versions on upgraded graphs.maintenance.rscovering internal compaction, absent_graph_commits, auto-cleanup stripping on internal tables, and the same on data tables; the cost LOCK is now green on the authenticated write path.Confidence Score: 5/5
Safe to merge — compaction is content-preserving, internal tables are read at HEAD so no manifest publish is needed, the bounded retry loop correctly handles concurrent writers, and the existence guard prevents failures on older graphs without commit-graph tables.
All changes are on the maintenance path (
optimize), not the live write or read path. Compaction preserves all versions (no GC), the auto-cleanup strip prevents the one destructive upgrade hazard on older graphs, and the newcompact_internal_tablepath is thoroughly tested.The retry loop in
compact_internal_tableinoptimize.rsis worth a second look:plan_compactionerrors exit the loop entirely via?rather than going through the retryable-conflict check, which is an asymmetry with the surrounding retry logic.Important Files Changed
compact_internal_table,clear_stale_auto_cleanup_config, andis_retryable_lance_conflict; wires all three internal system tables intooptimize_all_tables; removes the empty-table-tasks early return. The retry loop correctly handlesclear_stale_auto_cleanup_configandcompact_filesconflicts, butplan_compactionerrors exit the retry loop entirely via?. Multiple coordinator refreshes (one per internal table instead of one at the end) are correct but redundant._graph_commitstolerance, stale auto-cleanup config stripping on internal tables, and the same on data tables; updatesoptimize_on_empty_graphto assert 7 stats. Comprehensive coverage.internal_table_scans_are_flat_in_historyLOCK, switches to actorful writes viacommit_many_as/measure_insert_asso_graph_commit_actorsis exercised, and insertsdb.optimize()at each depth checkpoint to prove per-write scan is flat on a periodically-compacted graph.graph_commit_actors_uripromoted fromfntopub(crate) fnsooptimize.rscan call it directly.manifest_urifromlayoutaspub(crate)sooptimize.rscan reference it without going through the internallayoutmodule path.manifest_uripromoted frompub(super)topub(crate)to support the re-export inmanifest.rs.measure_insert_ashelper for the authenticated write path, mirroringmeasure_insertbut passing an actor so_graph_commit_actorsis exercised in IO cost measurements.commit_many_ashelper for building history on the authenticated write path, mirroringcommit_many.optimizenow covers internal tables and that the version chain still grows until step 2b lands.write_cost.rsrow to reflect the LOCK is now green every-PR and describes the compaction-at-depth-checkpoint pattern.Comments Outside Diff (1)
crates/omnigraph/src/db/omnigraph/optimize.rs, line 131-132 (link)committeddoc-comment says "published to__manifest", but for internal tablescommitted = trueonly means Lance advanced its HEAD — there is no manifest publish for these tables. Callers who readcommittedas "a manifest entry changed" will be confused for the two newtable_keyvalues. Consider a short clarifying note here.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (6): Last reviewed commit: "Merge branch 'main' into rfc-013-interna..." | Re-trigger Greptile