Skip to content

Make dataset in StorageBase interior-mutable - #236

Open
lucyge2022 wants to merge 10 commits into
lance-format:mainfrom
lucyge2022:issue198
Open

Make dataset in StorageBase interior-mutable#236
lucyge2022 wants to merge 10 commits into
lance-format:mainfrom
lucyge2022:issue198

Conversation

@lucyge2022

@lucyge2022 lucyge2022 commented Aug 10, 2026

Copy link
Copy Markdown

Enhancement on #198 - StorageBase should not own Dataset by value, and remove the necessity of using &mut self everywhere that needs a writer mutex for StorageBase bcos of that reason.

Root cause: Dataset rebinding, not the work itself

StorageBase owned its Dataset by value. Lance mutating ops (append, compact_files, checkout_latest, schema evolution) take &mut self not because they mutate in place, but because they replace the whole handle at the end:

*self = new_dataset;

That requirement propagated outward: append_merged_batchescommit_mergecleanup_own_shard → callers holding RwLock::write() on the store for the entire merge — including the expensive prepare phase (seal + read every flushed generation from object storage).

But this is an LSM: a merge folds sealed generations into the base table while add writes the active memtable at the WAL tail. Disjoint data. They should not have serialized for the borrow checker's sake.

Fix: interior-mutable dataset + narrow merge lock

piece change
StorageBase::dataset ArcSwap<Dataset> — merge/compact/reload publish a new handle via set_dataset() without &mut self on the store
merge / compact / schema evolution &self on StorageBase, RolloutStore, ContextStore, DatagenStore, GenericStore
merge vs merge explicit merge_lock (try_lock → second prepare no-ops) instead of accidental exclusivity from &mut
prepare / commit both run under shared store locks; PreparedMerge holds merge_lock until commit or drop

Sweeper, HTTP merge-wal, and rollout/context compact routes updated to read() for maintenance; no write lock spanning seal + generation reads + base append.

Ordering preserved

prepare_cleanup_merge still seals before reading the shard manifest. With deferred flush intervals, reading the manifest first leaves flushed_generations empty and rows durable but invisible until restart — load-bearing ordering kept intact.

What deliberately did not change

ContextStore still uses &mut self for delete/upsert and start_background_compaction — store-local lifecycle, unrelated to StorageBase's dataset handle. HTTP compact/merge paths do not need the outer write lock.

Theoretically prepare + commit two phase split could be removed, but out of scope for this PR.

Tests

Extended wal_merge_concurrency.rs (the existing generation-cleanup suite is explicitly serial — none of this had coverage):

  • appends succeed and no row is lost while a merge runs
  • a generation sealed during a merge is not dropped by the drain
  • concurrent merges do not duplicate rows; second prepare loses merge_lock while first PreparedMerge is live
  • appends still flow while PreparedMerge is held
  • an interrupted merge loses nothing; the next merge converges
  • an append is not blocked for the merge's duration
  • an append is not blocked during base-table compact (5s timeout; no wall-clock ratio vs compact — avoids flaky CI)

Each merge test drives the sweeper's exact discipline: read() prepare → read() commit via merge_like_sweeper.

Follow-ups (not folded in)

  • datagen_store.rs may still carry the older claim_epoch-then-close pattern — separate pass.
  • Context delete/upsert paths still take server-side write locks for correctness, not for dataset rebinding.

Verification

  • cargo test -p lance-context-core --test wal_merge_concurrency
  • cargo test -p lance-context-core --test wal_merge_generation_cleanup
  • cargo fmt --check and clippy --workspace --all-targets -D warnings

Closes #198.

@lucyge2022 lucyge2022 changed the title Issue198 Make dataset in StorageBase interior-mutable Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RolloutStore::dataset should be interior-mutable so merge/compact can refresh the handle without an exclusive lock

1 participant