From 41f5526c94cd3c7f3789026512b54348f55dc329 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Mon, 12 Oct 2026 09:13:41 -0700 Subject: [PATCH 1/3] perf(catalog): probe the next __manifest version by name instead of listing A warm read learned whether its branch's `__manifest` moved through Lance's `latest_version_id`, which lists `_versions/`. On a store with ordered listing that one request returns up to 1,000 keys, so a branch past 1,000 commits paid a full page on every read. The freshness probe now HEADs the attached versions above the one the handle holds, at most two, and treats the first "not found" as the end of the history. Lance writes an attached version only with a put-if-absent over a read of the version below it, and nothing deletes a version of a live branch's `__manifest`. A longer gap, a detached handle or any other error falls back to the listing. --- ...ss-probe-heads-next-version.performance.md | 7 ++ crates/omnigraph-catalog/src/lib.rs | 45 ++++++++- crates/omnigraph-catalog/src/tests.rs | 95 ++++++++++++++++++- docs/dev/lance.md | 1 + 4 files changed, 141 insertions(+), 7 deletions(-) create mode 100644 changelog.d/catalog-freshness-probe-heads-next-version.performance.md diff --git a/changelog.d/catalog-freshness-probe-heads-next-version.performance.md b/changelog.d/catalog-freshness-probe-heads-next-version.performance.md new file mode 100644 index 000000000..201610981 --- /dev/null +++ b/changelog.d/catalog-freshness-probe-heads-next-version.performance.md @@ -0,0 +1,7 @@ +- A read on a warm graph handle checks for new commits by asking for the next + `__manifest` version by name instead of listing the version directory. On + S3-compatible storage that listing returned up to 1,000 keys on every read + once a branch passed 1,000 commits; the check is now one small HEAD request + while nothing changed. See the [catalog freshness probe][freshness-probe]. + +[freshness-probe]: ../docs/dev/lance.md#current-compatibility-fences diff --git a/crates/omnigraph-catalog/src/lib.rs b/crates/omnigraph-catalog/src/lib.rs index 271730376..0460ffcd8 100644 --- a/crates/omnigraph-catalog/src/lib.rs +++ b/crates/omnigraph-catalog/src/lib.rs @@ -766,14 +766,52 @@ impl CapturedManifestProbe { } } +/// Versions above the held one the freshness probe asks for by name before +/// it lists `_versions/` instead. +const FRESHNESS_PROBE_STEPS: usize = 2; + +/// The latest attached version of `dataset`'s location (main or the branch +/// tree it is checked out on), asked for by name from the version it holds. +/// +/// `latest_version_id` lists `_versions/`. On a store whose listing is +/// ordered (S3, GCS, Azure) that is one request returning up to a thousand +/// keys however few of them are new, and a warm read pays it on every query. +/// Attached versions are contiguous: Lance writes version `v + 1` only with a +/// put-if-absent over a read of `v`, and nothing deletes a version of a live +/// branch's `__manifest`, so the first absent name above the held version +/// ends the history. (`cleanup` collects a retired branch's whole tree; the +/// named-branch probe's identifier read catches that.) A reclamation of old +/// `__manifest` versions must keep every version above one a live handle +/// holds. The probe HEADs `v + 1`, `v + 2`, +/// … for at most [`FRESHNESS_PROBE_STEPS`] names; a longer gap, a detached +/// handle or any answer other than "not found" falls back to the listing, +/// which stays the authority. Like the listing, it relies on the store +/// showing a completed write to a later request. +async fn latest_attached_version(dataset: &Dataset) -> lance::Result { + let held = dataset.version().version; + if !lance_table::format::is_detached_version(held) { + let scheme = dataset.manifest_location().naming_scheme; + let base = dataset.branch_location().path; + let store = dataset.object_store(None).await?; + let mut latest = held; + for _ in 0..FRESHNESS_PROBE_STEPS { + match store.exists(&scheme.manifest_path(&base, latest + 1)).await { + Ok(true) => latest += 1, + Ok(false) => return Ok(latest), + Err(_) => break, + } + } + } + dataset.latest_version_id().await +} + async fn probe_dataset_latest_incarnation( dataset: &Dataset, active_branch: Option<&str>, ) -> Result { if active_branch.is_none() { return Ok(ManifestIncarnation { - version: dataset - .latest_version_id() + version: latest_attached_version(dataset) .await .map_err(OmniError::storage)?, e_tag: dataset.manifest_location().e_tag.clone(), @@ -790,8 +828,7 @@ async fn probe_dataset_latest_incarnation( // yields the replacement identifier rather than a false match to the held // lifetime. let held = async { - let version = dataset - .latest_version_id() + let version = latest_attached_version(dataset) .await .map_err(|error| branch_ref_error(error, branch))?; let native = dataset.manifest().branch.as_deref().ok_or_else(|| { diff --git a/crates/omnigraph-catalog/src/tests.rs b/crates/omnigraph-catalog/src/tests.rs index 5f81760db..71811bfdd 100644 --- a/crates/omnigraph-catalog/src/tests.rs +++ b/crates/omnigraph-catalog/src/tests.rs @@ -8688,15 +8688,17 @@ impl lance::io::WrappingObjectStore for SmallScanProbeMarker { } } -/// Counts the requests `__history` is sent by verb, "not found" GETs included -/// (Lance's tracker records only successful GETs); with `refuse_suffix` a -/// suffix range is refused before it is sent, as `object_store`'s Azure client does. +/// Counts the requests a wrapped store (`__history` through [`Self::probes`]) +/// is sent by verb, "not found" GETs and HEADs included (Lance's tracker +/// records only successful GETs); with `refuse_suffix` a suffix range is +/// refused before it is sent, as `object_store`'s Azure client does. #[derive(Debug, Default, Clone)] pub(super) struct HistoryGets { refuse_suffix: bool, sent: Arc, absent: Arc, heads: Arc, + lists: Arc, } impl HistoryGets { @@ -8716,6 +8718,11 @@ impl HistoryGets { }; (take(&self.sent), take(&self.absent), take(&self.heads)) } + + /// The listings since the last call. + pub(super) fn drain_lists(&self) -> usize { + self.lists.swap(0, std::sync::atomic::Ordering::Relaxed) + } } impl lance::io::WrappingObjectStore for HistoryGets { @@ -8801,6 +8808,9 @@ impl object_store::ObjectStore for HistoryGetStore { &self, prefix: Option<&object_store::path::Path>, ) -> futures::stream::BoxStream<'static, object_store::Result> { + self.gets + .lists + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); self.original.list(prefix) } @@ -8808,6 +8818,9 @@ impl object_store::ObjectStore for HistoryGetStore { &self, prefix: Option<&object_store::path::Path>, ) -> object_store::Result { + self.gets + .lists + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); self.original.list_with_delimiter(prefix).await } @@ -8821,6 +8834,82 @@ impl object_store::ObjectStore for HistoryGetStore { } } +/// The freshness probe asks for the versions after the held one by name. +/// `latest_version_id` lists `_versions/`, and on a store whose listing is +/// ordered (S3, and Lance's in-memory stores) that one request carries up to +/// a thousand keys on every warm read. +#[tokio::test] +async fn freshness_probe_asks_for_the_next_version_by_name() { + use arrow_array::ArrayRef; + async fn commit(uri: &str, mode: WriteMode, enable_v2_manifest_paths: bool) { + let batch = RecordBatch::try_from_iter([( + "value", + Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef, + )]) + .unwrap(); + let schema = batch.schema(); + Dataset::write( + RecordBatchIterator::new(vec![Ok(batch)], schema), + uri, + Some(WriteParams { + mode, + enable_v2_manifest_paths, + skip_auto_cleanup: true, + ..Default::default() + }), + ) + .await + .unwrap(); + } + for v2_paths in [true, false] { + let uri = format!("shared-memory://freshness-{}/__manifest", ulid::Ulid::new()); + let append = || commit(&uri, WriteMode::Append, v2_paths); + commit(&uri, WriteMode::Create, v2_paths).await; + let sent = HistoryGets::default(); + let held = DatasetBuilder::from_uri(&uri) + .with_store_params(lance::io::ObjectStoreParams { + object_store_wrapper: Some(Arc::new(sent.clone())), + ..Default::default() + }) + .load() + .await + .unwrap(); + assert_eq!(held.version().version, 1); + sent.drain(); + sent.drain_lists(); + + // Unchanged: one HEAD of version 2, answered "not found". + let latest = probe_dataset_latest_incarnation(&held, None).await.unwrap(); + assert_eq!(latest.version, 1, "v2_paths={v2_paths}"); + assert_eq!(sent.drain(), (0, 1, 1), "v2_paths={v2_paths}"); + assert_eq!(sent.drain_lists(), 0, "v2_paths={v2_paths}"); + + // Another writer commits once: version 2 is found, 3 is absent. + append().await; + let latest = probe_dataset_latest_incarnation(&held, None).await.unwrap(); + assert_eq!(latest.version, 2, "v2_paths={v2_paths}"); + assert_eq!(sent.drain(), (0, 1, 2), "v2_paths={v2_paths}"); + assert_eq!(sent.drain_lists(), 0, "v2_paths={v2_paths}"); + + // A gap longer than the probe's steps: the listing answers. + for _ in 0..FRESHNESS_PROBE_STEPS { + append().await; + } + let latest = probe_dataset_latest_incarnation(&held, None).await.unwrap(); + assert_eq!( + latest.version, + 1 + 1 + FRESHNESS_PROBE_STEPS as u64, + "v2_paths={v2_paths}" + ); + assert_eq!( + sent.drain(), + (0, 0, FRESHNESS_PROBE_STEPS), + "v2_paths={v2_paths}" + ); + assert_eq!(sent.drain_lists(), 1, "v2_paths={v2_paths}"); + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] struct SmallScanIo { requests: u64, diff --git a/docs/dev/lance.md b/docs/dev/lance.md index 2ebefed14..a2d1fdd28 100644 --- a/docs/dev/lance.md +++ b/docs/dev/lance.md @@ -179,6 +179,7 @@ history of dependency bumps. | Table forks | No table fork is created for a graph branch: a branch write stages detached on the dataset and native ref its inherited registration names. Lance's `create_branch` accepts a detached id as the parent but leaves the branch headless (a linear commit on it is refused), which is why the engine forks nothing. Forks created before v11 (`fork.{incarnation}.m{base_version}.{commit_ULID}`, `legacy` in the incarnation position) stay valid: a registration naming one opens there, `table_fork_owner` records ownership, and `cleanup` may reclaim an unreferenced one after its parsed owner incarnation is gone and tag, ancestry and age protections allow it. Canonical ULIDs and version numbers are required for that name-based liveness check; generated forks with malformed or `legacy` owner components stay retained. Names never establish table ownership. | `lance_surface_guards.rs`, `branching.rs`, metadata tests | | Detached commits | Every graph-table write is a Lance detached commit (`_versions/d{version}.manifest`), which never moves HEAD and needs no conflict pass, and it is the table's version for its whole life: nothing replays it linearly, no `Restore` is used, a graph table's linear HEAD stays at its `v1` create, and the registration's `omnigraph.last_linear_version` records where the linear history stopped. Every manifest names its transaction file `{read_version}-{uuid}.txn`, which is how the engine matches a pin's `transaction_uuid` and follows a chain of detached commits by `read_version` links without a second request (the collector, `walk_chain`); the pure-insert merge proof follows the same links and also reads each link's transaction file to certify it. A detached commit never triggers Lance auto-cleanup: `auto_cleanup_hook` runs only from `commit_transaction`, and every engine commit passes `skip_auto_cleanup: true`. | `lance_surface_guards.rs` (`rfc_0067_*`), `collector.rs` tests | | Catalog publication | Every `__manifest` publish is an `Operation::Overwrite` committed through `CommitBuilder::with_max_retries(0)`. Lance 11 treats that as a strict overwrite: it skips conflict resolution and writes `_versions/{read_version + 1}.manifest` only if absent, so a concurrent commit fails the publish with `CommitConflict` instead of being rebased over; the publisher maps that to `RowLevelCasContention` and retries from a fresh load. A Lance release that rebased overwrites under zero retries would break this version CAS. | `omnigraph-catalog` `stale_overwrite_loses_the_version_cas_without_replacing_the_winner` (the stale publish itself), `publisher_retry_vocabulary_is_the_version_cas` (the error mapping) | +| Catalog freshness probe | A read on a warm handle learns whether its branch's `__manifest` moved by asking for the attached versions above the one it holds by name (`ManifestNamingScheme::manifest_path`, V1 or V2 paths): at most `FRESHNESS_PROBE_STEPS` (2) HEADs, the first "not found" ending the history. Lance 11's `latest_version_id` lists `_versions/` instead, and on a store with ordered listing (S3, GCS, Azure) that one request returns up to 1,000 keys, so a branch past 1,000 commits paid a full page on every read. The first absent name is the latest because Lance writes an attached version only with a put-if-absent over a read of the version below it, and nothing deletes a version of a live branch's `__manifest` (`cleanup` collects a retired branch's whole tree, which the named-branch probe's identifier read catches); a reclamation of old `__manifest` versions must keep every version above one a live handle holds. A longer gap, a detached handle or any answer other than "not found" falls back to the listing. | `omnigraph-catalog` `freshness_probe_asks_for_the_next_version_by_name` (V1 and V2 paths on an ordered store: a HEAD and no listing, then the listing past the step bound) | | Packed catalog record | Since stamp v12 the `__manifest` record fields are one `Struct` column with `lance-encoding:packed = true`, which Lance 2.1+ stores row-major in one column (`PackedStructVariablePerValueEncoder`), so a scan reads one column's pages for the record. Two Lance 11 limits shape the layout: a packed struct child with a null is refused at write (`Per-value compression not yet supported for block type Nullable`, the row format has no per-child null bit), so every child is non-null and the `present` bitmask child carries the logical nulls; and projecting a fixed-width child alone fails to decode (`invalid variable-width layout for UInt64`), so every scan projects `record` whole (no scan filters on a record field). Drop `present` at the bump whose packed struct accepts null values in a child, and the whole-record projection at the bump that decodes a child alone. | `omnigraph-catalog` `packed_record_round_trips_nulls_through_present_bits`, `lance_surface_guards.rs::packed_struct_refuses_null_values_and_lone_child_projection_lance_11` (goes red at either bump) | | Ordered walks | Lance 11's `HardCapBatchSizeExec` measures a one-row slice by its whole parent buffer, and `FilteredReadExec`'s byte rechunk (`rechunk_stream_by_size`, no copy) re-slices a batch's tail down to such slices, so sorting complete rows fails on ordinary multi-KiB rows as well as on a row wider than the 37.5 MiB sorter cap. Every `id`-ordered walk (branch merge, change feed, export) sorts only `id`, `_rowid` and `_rowaddr` and hydrates rows in bounded chunks (`ordered_cursor.rs`). Lance 12.0.0 deep-copies the one-row slice before measuring it (lance#9048); the tail collapse remains (lance#9692). | `changes.rs`, `export.rs` and `merge_fast_forward.rs` wide-row tests (`tests/helpers/wide_rows.rs`) | | MemWAL | Upstream support exists, but OmniGraph's RFC 0018 and RFC 0026 experiments were removed. No stream profile, token ledger, hidden stream column, or `_mem_wal` path is current. | `lifecycle.rs`, cluster removed-field diagnostics | From b28c3c9722aee2a78ee5d29c400493b5a020b610 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Mon, 12 Oct 2026 16:05:41 -0700 Subject: [PATCH 2/3] fix(catalog): confirm the held version before trusting an absent successor A removed or unreadable `__manifest` answers "not found" for every name, so the probe read it as "unchanged" and a write on a warm handle went on to recreate `__manifest/` instead of refusing before effects. The probe now HEADs the held version and the next one together and accepts "no change" only while the held version answers; otherwise the listing reports the failure as before. The warm-read cost tests pin the one recorded `__manifest` request, the HEAD of the held version, and the DST cost golden records the listings the probe no longer makes. --- ...ss-probe-heads-next-version.performance.md | 11 +-- crates/omnigraph-catalog/src/lib.rs | 41 ++++----- crates/omnigraph-catalog/src/tests.rs | 53 +++++++----- crates/omnigraph-dst/cost_table.txt | 69 +++++++-------- .../tests/integration/warm_read_cost.rs | 85 +++++++++++++------ docs/dev/lance.md | 2 +- 6 files changed, 153 insertions(+), 108 deletions(-) diff --git a/changelog.d/catalog-freshness-probe-heads-next-version.performance.md b/changelog.d/catalog-freshness-probe-heads-next-version.performance.md index 201610981..0a9273881 100644 --- a/changelog.d/catalog-freshness-probe-heads-next-version.performance.md +++ b/changelog.d/catalog-freshness-probe-heads-next-version.performance.md @@ -1,7 +1,8 @@ -- A read on a warm graph handle checks for new commits by asking for the next - `__manifest` version by name instead of listing the version directory. On - S3-compatible storage that listing returned up to 1,000 keys on every read - once a branch passed 1,000 commits; the check is now one small HEAD request - while nothing changed. See the [catalog freshness probe][freshness-probe]. +- A read on a warm graph handle checks for new commits by asking for its + `__manifest` version and the next one by name instead of listing the version + directory. On S3-compatible storage that listing returned up to 1,000 keys + on every read once a branch passed 1,000 commits; the check is now two small + HEAD requests sent together while nothing changed. See the + [catalog freshness probe][freshness-probe]. [freshness-probe]: ../docs/dev/lance.md#current-compatibility-fences diff --git a/crates/omnigraph-catalog/src/lib.rs b/crates/omnigraph-catalog/src/lib.rs index 2788d319f..4b55e87b9 100644 --- a/crates/omnigraph-catalog/src/lib.rs +++ b/crates/omnigraph-catalog/src/lib.rs @@ -829,10 +829,6 @@ impl CapturedManifestProbe { } } -/// Versions above the held one the freshness probe asks for by name before -/// it lists `_versions/` instead. -const FRESHNESS_PROBE_STEPS: usize = 2; - /// The latest attached version of `dataset`'s location (main or the branch /// tree it is checked out on), asked for by name from the version it holds. /// @@ -841,28 +837,35 @@ const FRESHNESS_PROBE_STEPS: usize = 2; /// keys however few of them are new, and a warm read pays it on every query. /// Attached versions are contiguous: Lance writes version `v + 1` only with a /// put-if-absent over a read of `v`, and nothing deletes a version of a live -/// branch's `__manifest`, so the first absent name above the held version -/// ends the history. (`cleanup` collects a retired branch's whole tree; the -/// named-branch probe's identifier read catches that.) A reclamation of old -/// `__manifest` versions must keep every version above one a live handle -/// holds. The probe HEADs `v + 1`, `v + 2`, -/// … for at most [`FRESHNESS_PROBE_STEPS`] names; a longer gap, a detached -/// handle or any answer other than "not found" falls back to the listing, -/// which stays the authority. Like the listing, it relies on the store -/// showing a completed write to a later request. +/// branch's `__manifest`, so while the held version `v` exists the first +/// absent name above it ends the history. (`cleanup` collects a retired +/// branch's whole tree; the named-branch probe's identifier read catches +/// that.) A reclamation of old `__manifest` versions must keep every version +/// above one a live handle holds. +/// +/// The probe HEADs `v` and `v + 1` together, then `v + 2` when `v + 1` +/// exists. `v` must answer: a removed or unreadable `__manifest` answers "not +/// found" for every name, and only the listing reports it as the failure it +/// is. A gap of more than two versions, a detached handle, an absent `v` or +/// any error falls back to the listing, which stays the authority. Like the +/// listing, the probe relies on the store showing a completed write to a +/// later request. async fn latest_attached_version(dataset: &Dataset) -> lance::Result { let held = dataset.version().version; if !lance_table::format::is_detached_version(held) { let scheme = dataset.manifest_location().naming_scheme; let base = dataset.branch_location().path; let store = dataset.object_store(None).await?; - let mut latest = held; - for _ in 0..FRESHNESS_PROBE_STEPS { - match store.exists(&scheme.manifest_path(&base, latest + 1)).await { - Ok(true) => latest += 1, - Ok(false) => return Ok(latest), - Err(_) => break, + let [at, next, after] = [held, held + 1, held + 2].map(|v| scheme.manifest_path(&base, v)); + let (held_present, next_present) = futures::join!(store.exists(&at), store.exists(&next)); + match (held_present, next_present) { + (Ok(true), Ok(false)) => return Ok(held), + (Ok(true), Ok(true)) => { + if let Ok(false) = store.exists(&after).await { + return Ok(held + 1); + } } + _ => {} } } dataset.latest_version_id().await diff --git a/crates/omnigraph-catalog/src/tests.rs b/crates/omnigraph-catalog/src/tests.rs index 0d6ce06bb..1a8303e15 100644 --- a/crates/omnigraph-catalog/src/tests.rs +++ b/crates/omnigraph-catalog/src/tests.rs @@ -9669,10 +9669,11 @@ impl object_store::ObjectStore for HistoryGetStore { } } -/// The freshness probe asks for the versions after the held one by name. -/// `latest_version_id` lists `_versions/`, and on a store whose listing is -/// ordered (S3, and Lance's in-memory stores) that one request carries up to -/// a thousand keys on every warm read. +/// The freshness probe asks for the held version and the ones after it by +/// name. `latest_version_id` lists `_versions/`, and on a store whose listing +/// is ordered (S3, and Lance's in-memory stores) that one request carries up +/// to a thousand keys on every warm read. A removed `__manifest` still fails +/// the probe: every name is absent there, so the listing answers. #[tokio::test] async fn freshness_probe_asks_for_the_next_version_by_name() { use arrow_array::ArrayRef; @@ -9713,34 +9714,46 @@ async fn freshness_probe_asks_for_the_next_version_by_name() { sent.drain(); sent.drain_lists(); - // Unchanged: one HEAD of version 2, answered "not found". + // Unchanged: version 1 is found and version 2 is absent. let latest = probe_dataset_latest_incarnation(&held, None).await.unwrap(); assert_eq!(latest.version, 1, "v2_paths={v2_paths}"); - assert_eq!(sent.drain(), (0, 1, 1), "v2_paths={v2_paths}"); + assert_eq!(sent.drain(), (0, 1, 2), "v2_paths={v2_paths}"); assert_eq!(sent.drain_lists(), 0, "v2_paths={v2_paths}"); - // Another writer commits once: version 2 is found, 3 is absent. + // Another writer commits once: versions 1 and 2 are found, 3 is absent. append().await; let latest = probe_dataset_latest_incarnation(&held, None).await.unwrap(); assert_eq!(latest.version, 2, "v2_paths={v2_paths}"); - assert_eq!(sent.drain(), (0, 1, 2), "v2_paths={v2_paths}"); + assert_eq!(sent.drain(), (0, 1, 3), "v2_paths={v2_paths}"); assert_eq!(sent.drain_lists(), 0, "v2_paths={v2_paths}"); - // A gap longer than the probe's steps: the listing answers. - for _ in 0..FRESHNESS_PROBE_STEPS { - append().await; - } + // A gap of more than two versions: the listing answers. + append().await; + append().await; let latest = probe_dataset_latest_incarnation(&held, None).await.unwrap(); + assert_eq!(latest.version, 4, "v2_paths={v2_paths}"); + assert_eq!(sent.drain(), (0, 0, 3), "v2_paths={v2_paths}"); + assert_eq!(sent.drain_lists(), 1, "v2_paths={v2_paths}"); + + // A removed `__manifest`: no name answers, and the listing's "not + // found" fails the probe instead of reporting the held version. + held.object_store(None) + .await + .unwrap() + .remove_dir_all(held.branch_location().path) + .await + .unwrap(); + sent.drain(); + sent.drain_lists(); + let error = probe_dataset_latest_incarnation(&held, None) + .await + .unwrap_err(); assert_eq!( - latest.version, - 1 + 1 + FRESHNESS_PROBE_STEPS as u64, - "v2_paths={v2_paths}" - ); - assert_eq!( - sent.drain(), - (0, 0, FRESHNESS_PROBE_STEPS), - "v2_paths={v2_paths}" + error.storage_failure().map(|failure| failure.kind), + Some(StorageFailureKind::NotFound), + "v2_paths={v2_paths}: {error}" ); + assert_eq!(sent.drain(), (0, 2, 2), "v2_paths={v2_paths}"); assert_eq!(sent.drain_lists(), 1, "v2_paths={v2_paths}"); } } diff --git a/crates/omnigraph-dst/cost_table.txt b/crates/omnigraph-dst/cost_table.txt index 3520d83dd..2782d285e 100644 --- a/crates/omnigraph-dst/cost_table.txt +++ b/crates/omnigraph-dst/cost_table.txt @@ -1,66 +1,63 @@ -AddFriend l.get calls=110 -AddFriend l.list calls=16 +AddFriend l.get calls=117 +AddFriend l.list calls=9 AddFriend l.put calls=24 -BranchCreate l.get calls=4 -BranchCreate l.list calls=8 +BranchCreate l.get calls=6 +BranchCreate l.list calls=6 BranchCreate l.put calls=6 BranchDelete l.delete calls=1 BranchDelete l.get calls=31 BranchDelete l.list calls=6 BranchDelete l.put calls=2 Cleanup l.delete calls=20 -Cleanup l.get calls=179 -Cleanup l.list calls=172 -DeletePerson l.get calls=3 -DeletePerson l.list calls=3 -EnsureIndices l.get calls=4 -EnsureIndices l.list calls=2 -InsertLegacy l.get calls=16 -InsertLegacy l.list calls=11 +Cleanup l.get calls=187 +Cleanup l.list calls=164 +DeletePerson l.get calls=6 +EnsureIndices l.get calls=6 +InsertLegacy l.get calls=22 +InsertLegacy l.list calls=5 InsertLegacy l.put calls=18 -InsertV l.get calls=1 -InsertV l.list calls=3 +InsertV l.get calls=3 +InsertV l.list calls=1 InsertV l.put calls=6 Optimize a.exists calls=2 Optimize a.put calls=1 -Optimize l.get calls=54 -Optimize l.list calls=17 +Optimize l.get calls=63 +Optimize l.list calls=8 Optimize l.put calls=50 -RemoveFriendshipsFrom l.get calls=13 -RemoveFriendshipsFrom l.list calls=8 +RemoveFriendshipsFrom l.get calls=20 +RemoveFriendshipsFrom l.list calls=1 RemoveFriendshipsFrom l.put calls=5 -UpdateV l.get calls=4 -UpdateV l.list calls=4 +UpdateV l.get calls=8 _audit a.delete calls=1 _audit a.get calls=2 _audit a.list calls=2 _audit a.put_if_absent calls=1 -_audit l.get calls=724 -_audit l.list calls=113 -_close l.list calls=3 -_history l.list calls=30 +_audit l.get calls=745 +_audit l.list calls=92 +_close l.get calls=3 +_history l.get calls=30 _setup a.delete calls=3 _setup a.exists calls=2 _setup a.list calls=1 _setup a.put_if_absent calls=3 -_setup l.get calls=45 -_setup l.list calls=18 +_setup l.get calls=50 +_setup l.list calls=13 _setup l.put calls=35 _verify a.get calls=10 -_verify l.get calls=1425 -_verify l.list calls=429 +_verify l.get calls=1620 +_verify l.list calls=254 MergeTargetHistory2 l.delete calls=3 -MergeTargetHistory2 l.get calls=111 -MergeTargetHistory2 l.list calls=12 +MergeTargetHistory2 l.get calls=116 +MergeTargetHistory2 l.list calls=7 MergeTargetHistory2 l.put calls=12 MergeTargetHistory32 l.delete calls=3 -MergeTargetHistory32 l.get calls=111 -MergeTargetHistory32 l.list calls=12 +MergeTargetHistory32 l.get calls=116 +MergeTargetHistory32 l.list calls=7 MergeTargetHistory32 l.put calls=12 MergeBackAfterThreeWay l.delete calls=3 -MergeBackAfterThreeWay l.get calls=129 -MergeBackAfterThreeWay l.list calls=17 +MergeBackAfterThreeWay l.get calls=134 +MergeBackAfterThreeWay l.list calls=12 MergeBackAfterThreeWay l.put calls=12 -BranchWrite l.get calls=21 -BranchWrite l.list calls=5 +BranchWrite l.get calls=23 +BranchWrite l.list calls=3 BranchWrite l.put calls=6 diff --git a/crates/omnigraph/tests/integration/warm_read_cost.rs b/crates/omnigraph/tests/integration/warm_read_cost.rs index 25c9b73d2..fe5b00dc0 100644 --- a/crates/omnigraph/tests/integration/warm_read_cost.rs +++ b/crates/omnigraph/tests/integration/warm_read_cost.rs @@ -24,14 +24,46 @@ use crate::helpers::{ with_setting, with_traversal, }; -/// A warm same-branch read must do ZERO `__manifest` object-store reads and must -/// not open the commit graph, even at commit-history depth. Wrapped in +/// The one `__manifest` request a warm read's freshness probe leaves in the +/// tracker: its HEAD of the held version's manifest. Its HEAD of the next +/// version answers "not found", which the tracker does not record, and it +/// lists nothing; any other recorded request is a manifest open or scan. +fn assert_only_the_probe_head(manifest_reads: u64, context: &str) { + let reads = last_manifest_reads(); + assert_eq!(manifest_reads, 1, "{context}: {reads:#?}"); + assert!( + reads.len() == 1 && reads[0].contains("/_versions/") && reads[0].ends_with(".manifest"), + "{context}: the one `__manifest` request is the probe's HEAD of the held version: {reads:#?}" + ); +} + +/// A warm named-branch read's recorded `__manifest` requests: the branch's +/// `BranchContents` lifetime witness and the probe's HEAD of the held version +/// in the branch tree, never a manifest body. +fn assert_branch_witness_and_probe_head(reads: &[String]) { + assert_eq!(reads.len(), 2, "{reads:#?}"); + assert!( + reads + .iter() + .any(|read| read.contains("_refs/branches/feature") && read.ends_with(".json")), + "the branch-lifetime witness is read: {reads:#?}" + ); + assert!( + reads.iter().any(|read| read.contains("/tree/feature") + && read.contains("/_versions/") + && read.ends_with(".manifest")), + "the other request is the probe's HEAD of the held branch version: {reads:#?}" + ); +} + +/// A warm same-branch read must not open or scan `__manifest` and must not +/// open the commit graph, even at commit-history depth. Wrapped in /// `cost_harness`, so `manifest_reads` is ground truth: the warm-coordinator -/// freshness probe rides the long-lived handle (which now carries the tracker) and -/// is served from Lance's cached manifest at 0 store reads, so this `== 0` also -/// catches any future warm-handle scan a per-op tracker would miss. Fails before -/// Fix 1, where the read path re-opens a fresh coordinator and scans both internal -/// tables. +/// freshness probe rides the long-lived handle (which now carries the tracker), +/// and its HEAD of the held manifest version is the one request recorded, so +/// this also catches any future warm-handle scan a per-op tracker would miss. +/// Fails before Fix 1, where the read path re-opens a fresh coordinator and +/// scans both internal tables. #[tokio::test] async fn warm_same_branch_read_does_no_resolution_opens() { cost_harness(async { @@ -55,9 +87,9 @@ async fn warm_same_branch_read_does_no_resolution_opens() { // data table by location+version instead of via the namespace, so the // per-table __manifest scan is gone too. Pre-fix, each of these is a deep scan // of an internal table that grows with commit count. - assert_eq!( - io.manifest_reads, 0, - "warm same-branch read must not scan __manifest (resolution or per-table)" + assert_only_the_probe_head( + io.manifest_reads, + "warm same-branch read must not scan __manifest (resolution or per-table)", ); assert_eq!( io.version_probes, 1, @@ -78,7 +110,7 @@ async fn warm_same_branch_read_does_no_resolution_opens() { /// Reading a branch's storage-format stamp (`omnigraph snapshot`, the served /// `GET /snapshot`) costs what any warm read costs: one version probe and no -/// `__manifest` read. The stamp comes from the snapshot's own manifest version, +/// `__manifest` read beyond its HEAD. The stamp comes from the snapshot's own manifest version, /// so it describes the same graph version as the tables beside it, where the /// stamp was once read by resolving the branch again and opening `__manifest` /// at its newest version. @@ -95,7 +127,7 @@ async fn warm_stamp_read_costs_one_version_probe() { omnigraph_catalog::INTERNAL_MANIFEST_SCHEMA_VERSION ); assert_eq!(io.version_probes, 1, "{io:?}"); - assert_eq!(io.manifest_reads, 0, "{io:?}"); + assert_only_the_probe_head(io.manifest_reads, &format!("{io:?}")); assert_eq!(io.internal_open_count, 0, "{io:?}"); let (pair, io) = measure(async { @@ -114,7 +146,7 @@ async fn warm_stamp_read_costs_one_version_probe() { .graph_manifest_version() ); assert_eq!(io.version_probes, 1, "{io:?}"); - assert_eq!(io.manifest_reads, 0, "{io:?}"); + assert_only_the_probe_head(io.manifest_reads, &format!("{io:?}")); assert_eq!(io.internal_open_count, 0, "{io:?}"); }) .await; @@ -140,9 +172,9 @@ async fn multi_table_query_does_no_manifest_scans() { .await; out.unwrap(); - assert_eq!( - io.manifest_reads, 0, - "a multi-table read must not scan __manifest once per touched table" + assert_only_the_probe_head( + io.manifest_reads, + "a multi-table read must not scan __manifest once per touched table", ); }) .await; @@ -465,10 +497,8 @@ async fn warm_branch_read_uses_one_ref_witness_without_manifest_scan() { assert_eq!(io.version_probes, 1); assert_eq!(io.internal_open_count, 0); assert_eq!(io.manifest_scan_count, 0); - assert_eq!(io.manifest_reads, 1); - let reads = last_manifest_reads(); - assert_eq!(reads.len(), 1); - assert!(reads[0].contains("_refs/branches/feature") && reads[0].ends_with(".json")); + assert_eq!(io.manifest_reads, 2); + assert_branch_witness_and_probe_head(&last_manifest_reads()); // Write to the branch so its tables are branch-owned (under tree/feature). db.mutate( "feature", @@ -490,13 +520,14 @@ async fn warm_branch_read_uses_one_ref_witness_without_manifest_scan() { .await; out.unwrap(); - assert_eq!(io.manifest_reads, 1, "warm branch read must spend exactly one read on the native branch-lifetime witness; reads: {:#?}", last_manifest_reads()); - let reads = last_manifest_reads(); - assert_eq!(reads.len(), 1); - assert!( - reads[0].contains("_refs/branches/feature") && reads[0].ends_with(".json"), - "the sole warm named-branch read must be BranchContents, not a manifest body: {reads:#?}" + assert_eq!( + io.manifest_reads, + 2, + "warm branch read must spend one read on the native branch-lifetime witness and one \ + on the probe's HEAD; reads: {:#?}", + last_manifest_reads() ); + assert_branch_witness_and_probe_head(&last_manifest_reads()); assert_eq!( io.version_probes, 1, "warm branch read probes only the requested branch once" @@ -1473,7 +1504,7 @@ async fn repeat_warm_read_reuses_table_handles() { warm.data_reads, 0, "a warm repeat read must reuse the held handle (0 table opens)" ); - assert_eq!(warm.manifest_reads, 0, "warm repeat read: 0 manifest opens"); + assert_only_the_probe_head(warm.manifest_reads, "warm repeat read: 0 manifest opens"); assert_eq!( warm.version_probes, 1, "warm repeat read: exactly one version probe" diff --git a/docs/dev/lance.md b/docs/dev/lance.md index 3c004feff..75da0b277 100644 --- a/docs/dev/lance.md +++ b/docs/dev/lance.md @@ -187,7 +187,7 @@ history of dependency bumps. | Table forks | No table fork is created for a graph branch: a branch write stages detached on the dataset and native ref its inherited registration names. Lance's `create_branch` accepts a detached id as the parent but leaves the branch headless (a linear commit on it is refused), which is why the engine forks nothing. Forks created before v11 (`fork.{incarnation}.m{base_version}.{commit_ULID}`, `legacy` in the incarnation position) stay valid: a registration naming one opens there, `table_fork_owner` records ownership, and `cleanup` may reclaim an unreferenced one after its parsed owner incarnation is gone and tag, ancestry and age protections allow it. Canonical ULIDs and version numbers are required for that name-based liveness check; generated forks with malformed or `legacy` owner components stay retained. Names never establish table ownership. | `lance_surface_guards.rs`, `integration/branching.rs`, metadata tests | | Detached commits | Every graph-table write is a Lance detached commit (`_versions/d{version}.manifest`), which never moves HEAD and needs no conflict pass, and it is the table's version for its whole life: nothing replays it linearly, no `Restore` is used, a graph table's linear HEAD stays at its `v1` create, and the registration's `omnigraph.last_linear_version` records where the linear history stopped. Every manifest names its transaction file `{read_version}-{uuid}.txn`, which is how the engine matches a pin's `transaction_uuid` and follows a chain of detached commits by `read_version` links without a second request (the collector, `walk_chain`); the pure-insert merge proof follows the same links and also reads each link's transaction file to certify it. A detached commit never triggers Lance auto-cleanup: `auto_cleanup_hook` runs only from `commit_transaction`, and every engine commit passes `skip_auto_cleanup: true`. | `lance_surface_guards.rs` (`rfc_0067_*`), `collector.rs` tests | | Catalog publication | Every `__manifest` publish is an `Operation::Overwrite` committed through `CommitBuilder::with_max_retries(0)`. Lance 11 treats that as a strict overwrite: it skips conflict resolution and writes `_versions/{read_version + 1}.manifest` only if absent, so a concurrent commit fails the publish with `CommitConflict` instead of being rebased over; the publisher maps that to `RowLevelCasContention` and retries from a fresh load. A Lance release that rebased overwrites under zero retries would break this version CAS. | `omnigraph-catalog` `stale_overwrite_loses_the_version_cas_without_replacing_the_winner` (the stale publish itself), `publisher_retry_vocabulary_is_the_version_cas` (the error mapping) | -| Catalog freshness probe | A read on a warm handle learns whether its branch's `__manifest` moved by asking for the attached versions above the one it holds by name (`ManifestNamingScheme::manifest_path`, V1 or V2 paths): at most `FRESHNESS_PROBE_STEPS` (2) HEADs, the first "not found" ending the history. Lance 11's `latest_version_id` lists `_versions/` instead, and on a store with ordered listing (S3, GCS, Azure) that one request returns up to 1,000 keys, so a branch past 1,000 commits paid a full page on every read. The first absent name is the latest because Lance writes an attached version only with a put-if-absent over a read of the version below it, and nothing deletes a version of a live branch's `__manifest` (`cleanup` collects a retired branch's whole tree, which the named-branch probe's identifier read catches); a reclamation of old `__manifest` versions must keep every version above one a live handle holds. A longer gap, a detached handle or any answer other than "not found" falls back to the listing. | `omnigraph-catalog` `freshness_probe_asks_for_the_next_version_by_name` (V1 and V2 paths on an ordered store: a HEAD and no listing, then the listing past the step bound) | +| Catalog freshness probe | A read on a warm handle learns whether its branch's `__manifest` moved by asking for attached versions by name (`ManifestNamingScheme::manifest_path`, V1 or V2 paths): the held version `v` and `v + 1` in two concurrent HEADs, then `v + 2` when `v + 1` exists; with `v` present, the first "not found" ends the history. Lance 11's `latest_version_id` lists `_versions/` instead, and on a store with ordered listing (S3, GCS, Azure) that one request returns up to 1,000 keys, so a branch past 1,000 commits paid a full page on every read. The first absent name is the latest because Lance writes an attached version only with a put-if-absent over a read of the version below it, and nothing deletes a version of a live branch's `__manifest` (`cleanup` collects a retired branch's whole tree, which the named-branch probe's identifier read catches); a reclamation of old `__manifest` versions must keep every version above one a live handle holds. An absent `v` (a removed `__manifest` answers "not found" for every name), a gap of more than two versions, a detached handle or any error falls back to the listing, which reports a removed `__manifest` as `NotFound`. | `omnigraph-catalog` `freshness_probe_asks_for_the_next_version_by_name` (V1 and V2 paths on an ordered store: HEADs and no listing, the listing past two versions, the listing's `NotFound` for a removed `__manifest`); engine `lifecycle::write_preparation_manifest_read_failures_carry_before_effect_evidence` (a write on a warm handle refuses before effects when `__manifest` is gone) | | Packed catalog record | Since stamp v12 the `__manifest` record fields are one `Struct` column with `lance-encoding:packed = true`, which Lance 2.1+ stores row-major in one column (`PackedStructVariablePerValueEncoder`), so a scan reads one column's pages for the record. Two Lance 11 limits shape the layout: a packed struct child with a null is refused at write (`Per-value compression not yet supported for block type Nullable`, the row format has no per-child null bit), so every child is non-null and the `present` bitmask child carries the logical nulls; and projecting a fixed-width child alone fails to decode (`invalid variable-width layout for UInt64`), so every scan projects `record` whole (no scan filters on a record field). Drop `present` at the bump whose packed struct accepts null values in a child, and the whole-record projection at the bump that decodes a child alone. | `omnigraph-catalog` `packed_record_round_trips_nulls_through_present_bits`, `lance_surface_guards.rs::packed_struct_refuses_null_values_and_lone_child_projection_lance_11` (goes red at either bump) | | Ordered walks | Lance 11's `HardCapBatchSizeExec` measures a one-row slice by its whole parent buffer, and `FilteredReadExec`'s byte rechunk (`rechunk_stream_by_size`, no copy) re-slices a batch's tail down to such slices, so sorting complete rows fails on ordinary multi-KiB rows as well as on a row wider than the 37.5 MiB sorter cap. Every `id`-ordered walk (branch merge, change feed, export) sorts only `id`, `_rowid` and `_rowaddr` and hydrates rows in bounded chunks (`ordered_cursor.rs`). Lance 12.0.0 deep-copies the one-row slice before measuring it (lance#9048); the tail collapse remains (lance#9692). | `integration/changes.rs`, `integration/export.rs` and `integration/merge_fast_forward.rs` wide-row tests (`tests/helpers/wide_rows.rs`) | | MemWAL | Upstream support exists, but OmniGraph's RFC 0018 and RFC 0026 experiments were removed. No stream profile, token ledger, hidden stream column, or `_mem_wal` path is current. | `integration/lifecycle.rs`, cluster removed-field diagnostics | From 82ec066bc16428b956590bbae25bca944c52ce9d Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Mon, 12 Oct 2026 16:28:29 -0700 Subject: [PATCH 3/3] test(gqt): count the freshness probe's held-version HEAD among a write's repeat reads A write probes its branch's freshness when it captures the branch and again when it revalidates the capture, and each probe HEADs the held `__manifest` version, so that HEAD repeats within a step beside the indexed table's manifest reopen. A read probes once. --- crates/omnigraph-gqt/tests/runner_dispatch.rs | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/crates/omnigraph-gqt/tests/runner_dispatch.rs b/crates/omnigraph-gqt/tests/runner_dispatch.rs index 49bbcbf30..3f5d7f3a9 100644 --- a/crates/omnigraph-gqt/tests/runner_dispatch.rs +++ b/crates/omnigraph-gqt/tests/runner_dispatch.rs @@ -1366,8 +1366,11 @@ fn measure_counts_schema_contract_requests_issue_817() { // first write, and Lance's detached commit loads the base's indexes. Behind // a strict insert's inline transaction the index section lies outside the // tail block the open read, and Lance opens the manifest again without the - // size it already learned: that HEAD is the one read a step may repeat. - let repeated_manifest_heads = |step: u64| -> u64 { + // size it already learned: that HEAD is one read a step may repeat. The + // other is the catalog freshness probe's HEAD of the held `__manifest` + // version, which a write makes when it captures its branch and again when + // it revalidates that capture. + let repeated_heads = |step: u64, class: &str| -> u64 { let group = measurements .iter() .find(|group| group["slot"] == "step" && group["step"] == step) @@ -1377,7 +1380,7 @@ fn measure_counts_schema_contract_requests_issue_817() { .as_array() .unwrap() .iter() - .filter(|request| request["class"] == "table_meta" && request["verb"] == "head") + .filter(|request| request["class"] == class && request["verb"] == "head") .filter(|request| !seen.insert(request["path"].as_str().unwrap().to_string())) .count() as u64 }; @@ -1389,14 +1392,20 @@ fn measure_counts_schema_contract_requests_issue_817() { ); assert_eq!( counts["repeat_reads"], - repeated_manifest_heads(step), + repeated_heads(step, "table_meta") + repeated_heads(step, "manifest_meta"), "step {step}: neither the inline contract nor table data repeats a read, \ - except the reopen of an indexed table's manifest" + except the reopen of an indexed table's manifest and the freshness \ + probe's HEAD of the held `__manifest` version" ); } assert_eq!( - repeated_manifest_heads(2), + repeated_heads(2, "table_meta"), 1, "the second insert reopens the first insert's manifest for its index section" ); + assert_eq!( + repeated_heads(3, "manifest_meta"), + 0, + "a read probes its branch's freshness once" + ); }