lance-format / lance-format/lance-context

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

Open
#198 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement
Dominant language
Rust
Stars
81
Forks
21
Avg merge
19h 23m
Merged PRs (30d)
9

Description

Context

Follow-up to the WAL-merge write-lock stall (a merge held the store's write lock for ~17s, blocking every concurrent add). That is being fixed by splitting the merge into a &self prepare phase (seal + read every flushed generation — the expensive part) and a short &mut self commit phase, so callers hold the exclusive lock only for the commit.

That fix removes the stall, but it works around the underlying constraint rather than removing it. This issue tracks removing it.

The constraint

RolloutStore owns its Dataset by value:

// crates/lance-context-core/src/rollout_store.rs
pub struct RolloutStore {
    dataset: Dataset,
    ...
}

Dataset's mutating operations are &mut self not because they mutate in place, but because they rebind the whole struct at the end:

// lance-7.0.0/src/dataset.rs:919
pub async fn append(&mut self, ...) -> Result<()> {
    let new_dataset = InsertBuilder::new(...).execute_stream(...).await?;
    *self = new_dataset;      // whole-struct replacement
    Ok(())
}

checkout_latest, add_columns and compact_files follow the same pattern. So the &mut requirement propagates outward — append_merged_batchescommit_mergemerge_own_shard_if_readycleanup_own_shard → and finally to every caller, which must take RwLock::write().

The result is that an exclusive lock is required for reasons that have nothing to do with mutual exclusion of the work itself. A merge appends to the base table and drains sealed generations; add writes the active memtable. They touch disjoint data. The lock exists only to satisfy the borrow checker.

Why this still bites after the split fix

The prepare/commit split shrinks the exclusive section but does not eliminate it, and it pushes the lock discipline onto callers:

  • Callers must remember the two-phase dance; a caller that just calls cleanup_own_shard() under one write lock silently reintroduces the stall. That is an easy mistake to make and there is no compile-time signal.
  • The commit phase still blocks appends for the duration of the base-table append. Small compared to reading N generations from object storage, but non-zero and proportional to merged data volume.
  • compact() has the identical shape (compact_files(&mut self.dataset, ...) then a reload) and still takes the write lock for its whole duration.

Proposal

Make the base dataset interior-mutable, e.g.:

dataset: Arc<RwLock<Dataset>>,   // or arc_swap::ArcSwap<Dataset>

Then merge, compact, and schema evolution can all take &self, refresh the shared handle when they commit, and never need an exclusive lock on the store. Merge-vs-merge exclusion (which is genuinely needed — two concurrent merges would read the same generations and append them twice) becomes an explicit, narrow mutex rather than a side effect of &mut.

ArcSwap is probably the better fit than RwLock: reads are the overwhelming majority, the value is cheap to clone (every Dataset field is already an Arc), and writers only ever replace the whole value.

Cost

~52 self.dataset call sites in rollout_store.rs, of which only 4 are assignments and 1 is a &mut borrow — the rest are reads (uri(), schema(), manifest(), object_store(), ...). Mechanical, but broad enough that it should not ride along with a targeted stall fix; it deserves its own review and its own soak.

ContextStore and DatagenStore have the same shape and would want the same treatment.

Acceptance

  • merge, compact and schema evolution take &self.
  • No caller needs RwLock::write() on the store to run a background maintenance task.
  • The existing concurrency tests (crates/lance-context-core/tests/wal_merge_concurrency.rs) still pass, plus a new one asserting compact does not block appends.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in crates/lance-context-core/src/rollout_store.rs and inspect the Dataset ownership and its roughly 52 call sites, then review the existing merge and compact paths. Compare the proposed shared-handle approaches against the current locking flow, and run crates/lance-context-core/tests/wal_merge_concurrency.rs. Done means merge, compact, and schema evolution take &self without store write locks, with a new compact-versus-append concurrency test.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend, databases
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.