Skip to content

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

Description

@beinan

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions