fix(mem_wal): read-your-writes via split index-apply and dual visibility cursors#1
Closed
hamersaw wants to merge 22 commits into
Closed
fix(mem_wal): read-your-writes via split index-apply and dual visibility cursors#1hamersaw wants to merge 22 commits into
hamersaw wants to merge 22 commits into
Conversation
…#7870) ## What `is_index_remap_caught_up` (used by `cleanup_frag_reuse_index`) reports an index as *not* caught up whenever its `dataset_version` predates a fragment-reuse (FRI) version — even when the index covers none of the fragments that version rewrote. On a table with more than one index this pins the fragment-reuse index forever: - a compaction with deferred remap rewrites fragments covered by index A but not index B, and writes an FRI version; - the remap catch-up remaps A and advances A's `dataset_version`, but B covers none of those fragments, so its remap is a no-op and B's `dataset_version` never advances; - `cleanup_frag_reuse_index` retains the version because B fails the version gate, even though B needs nothing; - the FRI (and the old fragments it references) can never be trimmed or garbage-collected, and any catch-up that fires while the FRI is non-empty re-runs forever making no progress. ## Fix Treat an index whose `fragment_bitmap` is disjoint from every fragment the reuse chain touches (old **and** new) as caught up, regardless of `dataset_version`: it holds no row address the FRI would remap, so trimming can never strand it (fragment ids are never reused). The disjointness set must include the **new** fragments, not only the old ones: a deferred-remap commit advances a *covering* index's bitmap onto the new fragments before its data is remapped, so an old-frag-only check would wrongly clear a still-stale index and trim a version it needs. Indexes that intersect the chain fall through to the existing conservative logic unchanged. ## Tests Adds `test_caught_up_uses_fragment_coverage_not_only_version` covering: - non-covering index with a stale version → caught up (the bug); - covering index still holding the old fragment → not caught up; - covering index advanced onto the new fragment but not yet remapped → not caught up (why new frags are in the check); - remapped index (version advanced) → caught up; - fragment removed outright (deletion) with an emptied index → caught up (not pinned forever). Verified the test fails without the fix. Existing `frag_reuse` tests pass (`--test-threads=1`).
This was referenced Jul 21, 2026
## Summary - remove FTS format v4 and make v3 the maximum supported capability version - use v3 for the code analyzer or 256-document posting blocks, while keeping text analysis with 128-document blocks on v2 - allow v3 with both 128- and 256-document blocks; reject code analysis on v1/v2 - update Rust, Python, Java, mem-wal handling, tests, and migration documentation ## Root cause The v4 bump conflated reader capability with a new physical file format. Code analyzer configuration is persisted in `InvertedIndexDetails` and changes token generation, not the posting or position schema. Configurable posting block size was already represented by v3 and persisted in metadata. For 128-document blocks, v3 uses the same fields and codecs as v2. Since FTS v3 has not been formally released, v3 can be the capability gate for both code analyzer metadata and 256-document posting blocks without introducing v4. ## Compatibility contract - v1/v2: 128-document posting blocks only; no code analyzer - v3: code analyzer supported; 128- and 256-document posting blocks supported - analyzer configuration: index details metadata - posting block size: physical encoding metadata - v4: unsupported; no alias or migration for the unreleased format ## Validation - `cargo fmt --all` - focused `lance-index` tokenizer, physical-layout, merge, metadata, and plugin tests - focused `lance` code-analyzer optimize, mem-wal metadata, and flush tests - `./mvnw -Dtest=InvertedIndexParamsTest test` - 17 focused Python FTS tests - `uv run make format` - `uv run make lint`
`InvertedIndexParams` is persisted in FTS index metadata and shared across Lance versions. PR lance-format#7681 added `deny_unknown_fields`, causing existing indexes that still contain retired fields such as `skip_merge` to fail to load. This also breaks `bench_regress` against its existing Wikipedia FTS index. Restore Serde's tolerant-reader behavior so historical and future metadata fields are ignored while validation for recognized parameters remains intact. Regression coverage exercises both training JSON and the on-disk `metadata.lance` loading path.
…ns can't reach target_k (lance-format#7869) ## Summary Creating an IVF index with `num_partitions` large enough to trigger hierarchical k-means (> 256) could fail badly when the training data can't be split into that many non-empty clusters (small dataset and/or many near-duplicate vectors): - Debug builds: hard panic at `debug_assert_eq!(heap.len(), target_k)` in `train_hierarchical_kmeans`. - Release builds: the assert compiles out, silently producing far fewer than `num_partitions` non-empty partitions, degrading recall with no error. This replaces the `debug_assert_eq!` with a returned `ArrowError::InvalidArgumentError` describing how many clusters could actually be formed, so both build types fail cleanly instead of panicking or silently degrading. The error propagates through `train_kmeans` → `do_train_ivf_model` as a normal `Result`. ## Test plan - Added `test_hierarchical_kmeans_too_few_distinct_vectors_errors`, which builds a heavily near-duplicated dataset with `num_partitions` (300) far exceeding the number of distinct vectors, and asserts a descriptive error is returned instead of a panic. - Verified this test panics against the old `debug_assert_eq!` code (reproducing the reported issue) and passes with the fix. - `cargo test -p lance-index --lib vector::` (318 tests) passes. - `cargo fmt --all -- --check` and `cargo clippy -p lance-index --tests --benches -- -D warnings` pass. Fixes lance-format#7867
Blob v2 exposes per-value range reads through `BlobFile`, while `Dataset.read_blobs` can batch only whole values. Python workloads that need row-specific windows therefore either schedule one call per blob or materialize complete values, losing centralized planning, bounded concurrency, and backpressure. This adds a dataset-level row-specific range contract for row IDs, indices, and addresses. Each request carries its row selector, offset, and length, so duplicates, out-of-order requests, and multiple ranges for one row remain unambiguous. Validation, physical-range planning, source grouping, coalescing, bounded scheduling, ordering, and error semantics stay in Rust; Python remains a thin collecting wrapper. ### Python user-path benchmark Real Amazon S3 validation ran in one CPython 3.12.13 process on a `c7i.4xlarge` in `us-east-2`. The workload used four 500 MiB Blob v2 values and 64 deterministic 100 KiB windows, with concurrency 16 and a 64 MiB I/O buffer. Both methods used the same requests and objects for ten repetitions, and method order rotated by repetition. The reported p50 and p95 use linear interpolation across those ten runs. The existing-path baseline used pre-opened `BlobFile` handles and a prewarmed 16-worker `ThreadPoolExecutor`, so handle lookup and executor startup were excluded in its favor. Its measured interval covered submitting 64 synchronous `BlobFile.read_range` calls through receiving every `bytes` result. The PR path measured one synchronous `Dataset.read_blob_ranges` call through receiving its collected result list. Payload validation ran outside both measured intervals. | Python user-path metric | Existing `ThreadPoolExecutor` + `BlobFile.read_range` | This PR: `Dataset.read_blob_ranges` | Benefit | | --- | ---: | ---: | ---: | | 64-range workload p50 wall-clock latency (lower is better) | 201.19 ms | 81.51 ms | 2.47x speedup | | 64-range workload p95 wall-clock latency (lower is better) | 377.65 ms | 103.74 ms | 3.64x speedup | | Logical throughput p50 (higher is better) | 31.07 MiB/s | 76.80 MiB/s | 2.47x throughput | ### Storage I/O diagnostic A separate Rust scheduler diagnostic at the same implementation commit measured the physical I/O for the same logical workload. It is used only to validate range efficiency, not as a Python latency baseline. | Storage path | Logical bytes | Physical bytes p50 | Physical requests p50 | Read amplification p50 | | --- | ---: | ---: | ---: | ---: | | `read_blob_ranges` | 6,553,600 | 6,556,844 | 72 | 1.0005x | | Whole-value `read_blobs` control | 6,553,600 | 2,097,155,244 | 136 | 320.0005x | The range API therefore reduced aggregate read amplification by 319.84x relative to the only existing dataset-level batch API. Direct S3 is intentionally omitted from the benefit table because it is a storage diagnostic control, not a Python user path. No client, OS, or S3 cache flush was performed; first-pass labels are observational and Amazon S3 server-side cache state is uncontrolled. Both measurements used the current PR head, `9c95e0324`.
This PR adds Python 3.14 support. - upgrade `duckdb` and `datasets` - duckdb/duckdb-python#143 - huggingface/datasets#7839 - fix `executor.rs` to properly propagate the runtime error with Python 3.14 (~~GIL-disabled~~) All tests passed with some warnings. ``` ❯ python -V Python 3.14.5 ❯ uv run make test ===================================================================================== warnings summary ====================================================================================== python/tests/test_dataset.py::test_sharded_iterator_fragments python/tests/test_dataset.py::test_sharded_iterator_batches python/tests/test_dataset.py::test_sharded_iterator_non_full_batch /home/moco/repo/mocobeta/worktrees/lance/calm-warbler/lance/python/python/lance/_dataset/sharded_batch_iterator.py:69: UserWarning: ShardedBatchIterator is deprecated, use :class:`Sampler` instead warnings.warn( python/tests/test_fork.py::test_table_roundtrip /home/moco/repo/mocobeta/worktrees/lance/calm-warbler/lance/python/python/lance/__init__.py:353: UserWarning: lance is not fork-safe. If you are using multiprocessing, use spawn or forkserver instead. warnings.warn( -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html ========================================================= 1101 passed, 594 skipped, 1 deselected, 4 warnings in 1396.62s (0:23:16) ========================================================== ``` [Benchmark result](lance-format#7728 (comment)) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **Chores** - Updated CI workflows and compatibility testing to use Python 3.14, including the Linux x86_64 wheel matrix and ARM job setup. - Adjusted wheel upload gating and refreshed test dependency constraints (datasets/duckdb), along with static analysis settings to target Python 3.14. - **Bug Fixes** - Improved background event pumping behavior during buffered-drain completion: pump errors are now propagated on Python 3.14+ (while older versions continue to log warnings without failing). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Weston Pace <weston.pace@gmail.com>
lance-format#7427) This PR introduces the concept of "index seeds". Indexes can opt-in to planting seeds during ingestion. The seeds are placed into the data files as global buffers. Later, when updating the index to include these data files, we can harvest the seeds instead of scanning the data itself. This is primarily intended to avoid a potentially expensive data scan to update the index. As an example this PR adds index seeds for wide (binary, fixed-size-list, string) columns when creating a zone map index. Now we calculate the min/max/nulls during ingestion, when the data is already present and flowing through the system. Then, when we go to update the index, all we are doing is reading back those counts and adding them to the index (instead of scanning the large column all over again). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Zone-map indexes can now use stored seed statistics to accelerate incremental updates. * Zone-map configuration now supports optional `rows-per-zone` and seed usage (`use-seeds`) with backward-compatible behavior when omitted. * When appending to existing datasets, the system can persist zone-map seed metadata for eligible indexes. * Added a benchmark to compare incremental update performance with and without seeds. * **Bug Fixes** * Incremental updates automatically fall back to full processing when seed data is unavailable or can’t be used. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ance-format#7872) ## Stack: 1/3 — foundational hardening This is the first of three stacked PRs that split the former lance-format#7791 (WAL poison / read-visibility / flush-interval) into reviewable pieces. It carries the changes that stand on their own, independent of the visibility redesign: - **perf(mem_wal): index small batches inline instead of one thread per index** — stop spawning a thread per index for tiny batches; index inline. - **fix(mem_wal): fail reads on a poisoned writer** — once a writer self-fences, reads must surface the failure instead of returning a silently truncated view. - **fix(mem_wal): reject index configs that disagree with the schema at shard open** — validate FTS/HNSW/BTree/PK column types and existence once at `open()`, before any row is accepted. A config that fails deterministically on every insert (including WAL replay) makes poison-and-replay non-terminating; reject it up front. Also errors (instead of silently indexing nothing) when an FTS column is missing from a batch, and relabels HNSW capacity-exhaustion as `internal` rather than `invalid_input`. Reviewable independently of the two PRs stacked on top. **Stack** 1. **this PR** — foundational hardening → `main` 2. #1 — read-visibility redesign → this branch 3. #2 — flush-interval ticker → PR 2's branch Splits the former lance-format#7791 into three reviewable pieces; lance-format#7791 can be closed once this stack lands. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t re-append After `replay_memtable_from_wal` rehydrated a memtable, the durability cursor stayed at "nothing flushed". The next WAL flush therefore re-covered `[0, end)`: it appended the already-durable rows to the WAL a second time *and* re-inserted every replayed row into the in-memory indexes. None of the three indexes is idempotent. HNSW mints fresh node ids for the same row, so KNN returns it twice and burns two of the `k` slots. FTS increments `doc_count`, `total_tokens`, and every term's `df` rather than recomputing them, corrupting BM25 for the whole memtable. BTree is a multiset whose second insert sets `pk_has_overrides` permanently, disabling HNSW plan selection and WAND pruning. A full scan kept looking healthy while every index-accelerated query silently returned duplicates — and it compounded, because the WAL now held those rows twice, so the next crash replayed both copies. Stamp the durability cursor at the end of replay: the batches came from the WAL, and `insert_batches` has just re-derived the indexes over them. The index cursor already advanced itself; only durability was missing. Regression test reproduces the original report: reopen + replay + one 2-row put grew the WAL from 8 rows to 18 instead of to 10. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`mark_wal_flushed` had no production callers. The two collections it maintained — `wal_batch_mapping` and `flushed_batch_positions` — were allocated per memtable and read only from tests, as was `MemTable::last_flushed_wal_entry_position` (production tracks that on `WriterState`, a different struct). `BatchStore:: is_wal_flush_complete` had no callers at all. What the L0 flush actually gates on is `MemTable::all_flushed_to_wal()`, which derives from the batch store's durability watermark and stays. The tests were calling `mark_wal_flushed` only to satisfy that precondition, so they now set the watermark directly — the same thing production does, instead of a parallel bookkeeping path that only tests could reach. The two tests that covered nothing but the deleted mapping are replaced by one that covers the surviving behaviour: `all_flushed_to_wal()` flips only once the watermark covers every committed batch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The durable watermark was meaningless across a memtable rotation. `WalFlusher` is built once per writer and its watch channel is never reset, but the targets put against it were *memtable-local* batch positions — and positions restart at 0 in every new memtable. So after memtable A flushed N batches (watermark = N), the first put into memtable B targeted position 1, saw N >= 1, and acked immediately with no WAL append at all. The first N puts into every post-rotation memtable were falsely acked as durable: `durable_write: true` silently degraded to non-durable. When B's append finally landed it sent a *smaller* value, walking the watermark backwards — so a watcher from A targeting N could miss N entirely and block until some later memtable climbed back to it. A hang, not just a stale read. Fix the coordinate system rather than the symptom: - `BatchStore` carries an immutable `global_offset`, stamped at freeze from the outgoing store's `global_end()`. It is a coordinate, not a cursor: it cannot restart and cannot move backwards. - The durability cursor moves onto `WalFlusher` as a writer-global exclusive count, and is the only thing the watch channel carries. It advances monotonically (`max`), so an out-of-order completion cannot walk it back. - `BatchStore::local_end(global_cursor)` is the single place the global-to-local subtraction is written. It saturates in both directions, and both are reachable: a cursor below a store's offset means "nothing here yet" — the ordinary state of a freshly rotated memtable — and open-coding the subtraction underflows there, which in release wraps to a huge end and makes the entire new memtable visible at once. - The per-memtable `max_flushed_batch_position` (inclusive, `usize::MAX`-sentinel) is deleted. `pending_wal_flush_*` and `all_flushed_to_wal` now derive from the global cursor. Two things fell out while wiring it up: - The put path captured `batch_store` *after* the freeze check that may rotate the memtable, pairing the new store with the old store's end position. Capture it before, next to the insert that produced those positions. - `flush_memtable` sampled the cursor before awaiting the WAL-append completion it depends on, so the L0 precondition saw a stale value. Sample it after. Regression test: with a local target, a post-rotation put acks at durable=2 while its own batch spans [2, 3) — acked, never appended. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`max_visible_batch_position` was an inclusive position starting at 0, so a cursor of 0 meant *both* "nothing is visible" and "batch 0 is visible". There was no sentinel and no `Option` to tell them apart. The window is real, not theoretical. `BatchStore::append` publishes `committed_len` on the put path, under the state lock, before the WAL flush that indexes the batch is even triggered — and that flush is a ~100ms S3 PUT on another task. So batch 0 of every memtable sat committed and readable for a full round-trip before it was indexed or durable, and only through the arms backed by the batch store: the index-backed arms, reading the same cursor, saw nothing. The tiers actively disagreed. Express the cursor as an exclusive count. `0` now means nothing, the `+1` disappears, `i < count` replaces `i <= max`, and the off-by-one becomes inexpressible. `checked_sub(1)` conversions at the consumers fall away — every site got simpler, as did `bounded_in_memory_membership`, whose `batch_count == 0` case now falls out of the arithmetic instead of being special-cased. Rename it to `indexed_count` while we are here. It has only ever been an *indexed* cursor — it is advanced at the end of `insert_batches`, once every index insert for a batch completes, and never before. Five read sites treated it as a visibility cursor, which is a separate thing that belongs to the writer. Deriving visibility from it is the next change; this one just stops the name from lying. Two things the conversion turned up: - `point_lookup` did `indexed_count().min(len - 1)`, reading the cursor as an inclusive index. It had no "nothing visible" case at all. - `test_shard_writer_e2e_correctness` passed *only* because of this bug: it wrote with `durable_write: false` and scanned, and the rows appeared because the un-advanced cursor of 0 was misread as "batch 0 is visible". A non-durable put is not yet read-your-writes — the index apply is welded to the WAL flush — so the test now writes durably, which is what it meant to test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Readers keyed off `indexed_count`, which the index insert advances itself. But the WAL append and the index apply run under one `tokio::join!`, and `join!` runs both arms to completion — it does not cancel the index arm when the append fails. So on a failed append the index arm still advanced the cursor every reader treats as visibility, publishing rows that are not in the WAL and that replay will not reproduce. `MemTableScanner::new` asserts the opposite in its own doc comment. Split the cursor in two. `indexed_count` stays the *apply* cursor: it tracks what the index layer has ingested, it is what HNSW's contiguity check needs, and it advances on a failed flush — which is fine, because indexes are derived state and replay rebuilds them. `visible_count` is new, and is the only thing readers snapshot: the writer advances it downstream of *both* arms succeeding, so `visible => durable` holds unconditionally. Also make an index-apply failure terminal. `insert_batches` joins every index thread unconditionally, so a failure leaves the others fully applied, and none of them can be rolled back: HNSW has no delete, FTS has already incremented its collection statistics, BTree has linked into an append-only skiplist. Both ways out are corrupt — retry re-covers the range and re-inserts into the indexes that did succeed; skip it and the failed index is permanently short rows the others have. So discard instead: poison, and let reopen rebuild the indexes from the WAL. The rollback primitive already exists and it is `open()`. Replay publishes explicitly: its batches are durable by construction and it bypasses the flush, so without it the recovered rows stay invisible. Regression tests pin both halves. With the index arm publishing, a row whose WAL append failed becomes readable (visible_count=1 where it must be 0). An index insert that fails deterministically now poisons the writer instead of leaving it to limp on with a corrupt index. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…the WAL flush The WAL append and the index apply were welded together under one `tokio::join!` on one schedule. They have nothing in common: one is an in-memory write measured in microseconds, the other a ~100ms S3 PUT billed per call. Batching exists to bound API cost, which an in-memory index apply does not incur — so the index was being dragged onto the WAL's schedule for no reason, and that is what made read-your-writes impossible without durability. Split them into two tasks with two channels, each a single sequential consumer: - The index-apply task is triggered per put, in both modes. Being a single consumer is the safety property: `HnswGraph::insert_batch` hard-rejects any range whose start is not its `indexed_len`, and a single consumer guarantees contiguous in-order ranges however many putters race behind it. Ordering comes from the task, not from a flush interval — so triggering per-put is exactly as safe as triggering on a timer, and a put becomes visible in milliseconds rather than waiting on an S3 round-trip. - The WAL-append task keeps the expensive work and is now append-only. The `join!` is gone, and with it the dirty read it caused: a failed append can no longer publish rows through an index arm that ran anyway. They need separate channels, not two message types on one: `TaskDispatcher::run` awaits `handle()` inline, so sharing would queue every index apply behind an S3 PUT. Visibility becomes derived rather than published. `WriterCursors` holds the writer-global `durable` count; each memtable's `IndexStore` holds its own `indexed` count; and `visible` is computed on demand as `min(indexed, durable)` under `durable_write`, or just `indexed` without it. Nothing caches it, which is deliberate: a cached `min` recomputed by two independent tasks is the classic store-buffer race — under Release/Acquire both tasks can read the other's pre-store value, both compute a minimum below the true one, and a max-clamped publish leaves it permanently short, hanging any put blocked on it. With nothing cached there is nothing to leave stale. The notify channel is a bare wake-up and every waiter recomputes. What this buys: `durable_write: false` now costs the caller durability *only*, not visibility. A non-durable put is read-your-writes through every arm, indexed ones included — previously the row sat unindexed in the batch store until some later flush happened along, so a full scan could find it while every index-accelerated query could not. Also: `close()` drains both tasks, `freeze` triggers the index apply for the outgoing store, and every memtable's `IndexStore` is bound to the cursors — including the empty one built when no indexes are configured, which would otherwise fall back to `visible == indexed` and publish before durability. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A single memtable holds at most `max_memtable_batches` batches, but a WAL is unbounded. Replay stuffed every WAL entry into one memtable, so a shard whose WAL had grown past one memtable's capacity failed `open()` outright with "MemTable batch store is full" — permanently unopenable. Replay now rotates exactly as the live write path does, on the same trigger. When the active memtable reaches the flush threshold it is sealed and — because the data is already durable in the WAL — flushed straight to a Lance generation with the same `MemTableFlusher::flush` the live path uses, rather than held in memory until open finishes. That bounds resident memory to ~two memtables and truncates the WAL as it goes, so a later reopen replays only the unflushed tail. Rotation is at WAL-entry boundaries, so each sealed generation covers a clean range of complete entries and stamps the last as its `replay_after_wal_entry_position`. The flush trigger is now one predicate, `memtable_reached_flush_threshold`, shared by the live path (post-insert, "room for one more batch?") and replay (pre-insert, "room for the next entry?"). It carries both criteria — `max_memtable_size` bytes and batch-store capacity. The byte trigger matters beyond avoiding overflow: it is what keeps a memtable under `max_memtable_rows`, and therefore keeps the in-memory HNSW index (sized to `max_memtable_rows`) from exhausting its capacity when the final active memtable is indexed. A single predicate is what stops the two paths from drifting — the exact class of bug this change set is about. `MemTable::is_batch_store_full` is deleted: its one production caller now goes through the shared predicate, and the two remaining test callers use the public `batch_store().is_full()` directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address CodeRabbit review feedback on the WAL visibility branch:
- validate_index_configs now rejects a single-column primary key on a
column absent from the schema, closing the gap where such a config
passed validation and then failed deterministically on every index
build and WAL replay. Existence is checked for all PK columns; the
order-preserving encodable-type check stays gated on composite keys.
- Restore the index-update statistics. Index application moved onto its
own task, leaving do_flush's record_index_update branch dead (gated on
a hardcoded false) so index_update_count/time/rows and
log_wal_breakdown() reported a permanent zero. apply_index_range now
returns IndexApplyStats { rows_indexed, duration } and
IndexApplyHandler records real applies, skipping coalesced no-ops. Drop
the dead do_flush gate and the vestigial index fields on WalFlushResult.
- Fix stale docs that still called indexed_count a durable visibility
watermark, and correct the point-lookup BTree test comment.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…or lock `check_poisoned` and `mark_terminal_failure` both `.unwrap()`ed the `terminal_error` lock, so a panic anywhere under it turned every later poison check into a panic of its own. Ignore the poisoning instead of reporting it. The guarded data cannot be torn: the sole writer builds the `WalFlushFailure` and assigns it whole under an `is_none()` check, so a panic mid-section leaves the slot exactly as it was. There is no invariant here for poisoning to protect — the flag is pure alarm. Mapping the poisoning to an error would be worse than the panic. This mutex exists to carry the reason a writer is fenced, and recovery is reopen -> replay driven by that `FenceReason`; answering "mutex was poisoned" buries it precisely when a caller needs it. It also has nowhere to go in `mark_terminal_failure`, which returns `()`. Test: latch a `PersistenceFailure`, poison the mutex from a panicking thread, then assert the typed reason and message still surface and the latch still keeps the first failure. It panics against the old code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three fixes from PR review, each confirmed by reproducing the failure in a test that fails on the pre-fix tree: - open(): derive PK metadata and run the durable-write/flush-interval and index-config validation *before* claim_epoch. These checks ran after the epoch was claimed (and, for a successor, after the predecessor was fenced), so an open doomed by purely local input knocked the healthy incumbent off the shard with a PeerClaimedEpoch fence. - validate_index_configs(): reject a config whose field_id names a different column than its `column`. Index selection keys off field_id alone (a single-column PK reuses the BTree whose field_id matches), so a mismatch bound the wrong index under a valid-looking name -- stale reads plus the wrong column flushed into the durable PK sidecar. Coupled with open()'s validation move because the signature and its only caller change together. - freeze_memtable(): retain the outgoing memtable in the read view before the fallible index-apply/WAL-flush dispatches, and poison on a failed send. The active memtable was already replaced, so a failed dispatch dropped the table and its accepted rows silently vanished -- a zero-row scan with no error, on a branch whose whole point is to poison instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…kipping it `apply_index_range` and `flush_from_batch_store` walked `[start, end)` and silently dropped any position `BatchStore::get` returned `None` for. The store is append-only (`get` returns `None` only past `committed_len`), so a hole means the caller asked to cover a batch that was never committed. Skipping it while still advancing the cursor was silent corruption: the WAL path advanced durability to `end_batch_position` even though a batch was never appended (lost on replay), and the index path advanced `indexed_count` past a never-indexed batch (rows counted visible but absent from every index). Both now error on a missing position, naming the range and committed length. The flush path returns a terminal (writer_poisoned) error so `flush` poisons the writer before durability moves; the index path's error poisons via its handler. Reopen replays the WAL. Holes are impossible today, but this fails loudly the day eviction lands rather than diverging silently. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hamersaw
force-pushed
the
feat/wal-read-visibility
branch
from
July 21, 2026 17:29
c99ce03 to
8e6dcea
Compare
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
Owner
Author
|
Superseded by lance-format#7888 — lance-format#7872 (PR-1) merged, so this branch was rebased onto |
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
7 participants
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.
Stack: 2/3 — WAL read-visibility redesign
The core of the former lance-format#7791. Rows become visible to readers only once they are both indexed and WAL-durable, resolved through two independent cursors instead of a single conflated watermark. Index application runs on its own task rather than as an arm of the WAL flush, giving read-your-writes in every mode.
Commits (bottom → top):
indexedcursor and a writer-global durability cursor; readers snapshotvisible_countderived from both.claim_epoch(a doomed open must not fence the incumbent); reject a config whosefield_idnames a different column than itscolumn; retain the outgoing memtable and poison on a failed freeze dispatch.Stack
mainfeat/wal-open-validation(PR 1's branch)Based on PR 1 and must merge after it.
🤖 Generated with Claude Code