doc: one execution path for index peeks

[antiguru]

Design document for unifying how a fast-path index peek is executed.

Before the stack below it, an inline peek walked the arrangement until it had collected a threshold of bytes, then discarded that work and deferred to the stashing path, which walked the same prefix again. The document argues for one scan type with two placements, and for treating the stash as a state transition of a single scan rather than a restart of a different one. It also records what the scan's budget may be denominated in: consumed cursor positions, not elapsed time.

The document sits at the top of the stack rather than the bottom, so it describes what the PRs beneath it actually built rather than what they were expected to build. Those are #38507 (bounding the unbudgeted parts of the scan), #38508 (PeekScan, one budgeted scan across both phases), #38509 (the offload, the permit and the tokio driver) and #38510 (the stash as a state transition of that same scan).

🤖 Opened by Claude Code on behalf of @antiguru

[antiguru]

Fact-check of the document against the implementation, posted by Claude Code on behalf of @antiguru. Checked at 4f1e43c7a6 against the implementation tree 3cd9c996a3, the stack base f9b255f6fc, upstream/main, and the three referenced branches for #38040, #38158 and #38429.

Nine wrong or unverifiable claims, plus smaller ones. The document was reconciled once already in eb0ee27bee; these survived it.

Critical

1. A quotation attributed to the code exists nowhere in the repo. Lines 65-68 put in quotation marks: "A fresh walk over the same trace: the iterator that produced UsePeekStash was consumed deciding that the result is too big to return inline." git grep for that string across f9b255f6fc, upstream/main and the #38429 branch returns exactly one hit, the design document itself. The base tree's UsePeekStash handling carries no such comment (f9b255f6fc:src/compute/src/compute_state.rs:1101-1123). The restart is real; the sentence quoted as evidence for it is not. Delete the quote or quote something that exists.

2. Cancellation state 2 is not a state. Lines 441-443: "Mid inline slice. The slice is bounded by the inline budget, so it finishes, and cancellation is observed before promotion." Nothing observes cancellation there. collect_finished_data returns PeekStatus::Promote(scan) and process_peek inserts PendingPeek::Offloaded and returns in the same worker turn (3cd9c996a3:src/compute/src/compute_state.rs:1259-1291), while handle_cancel_peek runs only on the command drain (:934-938). A cancel landing mid-slice collapses into state 4. There are four cancellation states, not five, and the most reassuring one is the fictional one.

3. The retention invariant is false for every non-streamable peek. Lines 287-291 state that each scan holds at most peek_response_stash_threshold_bytes and call it "an invariant of PeekScan, so it holds for a queued scan exactly as it holds for a running one". That is the load-bearing half of the "retention is monitored, not capped" argument. But batch_ready() is self.peek_stash_eligible && self.total_size > self.peek_stash_threshold_bytes (3cd9c996a3:src/compute/src/compute_state/peek_scan.rs:335-337), and eligibility is is_streamable, order_by.is_empty() && project == identity (src/expr/src/relation.rs:3552-3554). A peek with an ORDER BY or a non-identity projection is never batch-ready, is still promotable and queueable, and its prefix is bounded only by max_result_size, default 1 GB. With ORDER BY and no LIMIT, max_results is None so thinning never fires either. Secondary: even for eligible scans the bound is the threshold plus the row that crossed it, which peek_scan.rs:137-138 states and the document drops.

4. Layer 1's described content is not in layer 1. Lines 577-579 credit peek/fueled-iterator (#38505) with "#38040's PeekResultIterator::step(&mut fuel) -> Step and the clusterd-test-driver plumbing that exercises it". That branch is one commit, 01b6e4a1df, touching one file, peek_result_iterator.rs, +48/-16, and its own message says the surrounding machinery is out of scope. No clusterd-test-driver plumbing exists anywhere in the stack: git diff --stat f9b255f6fc 3cd9c996a3 touches no such path. The identical claim about #38040 at lines 602-604 is true, so this is #38040's content attributed to our own layer.

Important

5. "PendingPeek ends with fewer index-peek states than it has today" is false; it ends with the same number. Base: Index, Persist, Stash, so two index-peek states (f9b255f6fc:src/compute/src/compute_state.rs:1306-1314). Stack: Index, Persist, Offloaded, also two (3cd9c996a3:...:1548-1556). The "three PendingPeek states for one target type" at line 44 holds only by counting #38429's IndexOffloadPeek, which line 520 concedes is unmerged. The document runs two definitions of "today" and draws its headline trade-off from the more flattering one.

6. The Deleted list contains a non-change. Line 509-510 lists "snapshot_for_offload's spare oks_stash cursor" as deleted. git grep snapshot_for_offload 3cd9c996a3 -- src finds nothing; it exists only on the #38429 branch. Bullet 4 carries the right caveat for OffloadSnapshot and IndexOffloadPeek; this one does not, so a list read as a change set contains something the stack never had. Same defect at lines 98-99, "spawn_offloaded_walk already ships production cursors to another thread", present tense for a function that lives only on the unmerged branch.

7. The stack deletes a documented user-facing metric and the Deleted list omits it. git diff f9b255f6fc 3cd9c996a3 -- doc/user/data/metrics.yml removes mz_stashed_peek_seconds_{bucket,count,sum}. The Deleted list covers dyncfgs and Rust types only. A metric vanishing from the user-facing catalog matters more to an operator than PEEK_STASH_NUM_BATCHES, which is listed.

8. Cursor setup neither sums slices nor mixes substrates. Lines 429-433 claim the error scan, cursor setup, row iteration and result sort timers each sum the slices a walk was cut into and, for a promoted walk, mix worker time with the task's. cursor_setup_time is one Instant::now()/elapsed() pair around PeekResultIterator::new inside PeekScan::new (peek_scan.rs:216-228), and PeekScan::new is called only from the inline driver (compute_state.rs:1931-1938). One measurement, always on the worker. metrics.rs:233 repeats the same error in the metric help, so document and code are wrong together.

9. Present-tense claims about code the stack already replaced, outside the section scoped to the base tree. Lines 192-196, inside ### The scan, say IndexPeek::collect_finished_data "walks the error trace inline, in a while cursor.key_valid(&storage) loop with no bound". True of f9b255f6fc:...:1682-1705, false of 3cd9c996a3:...:1920-1971, where the same method opens a PeekScan. The base-tree disclaimer at lines 51-54 covers "What exists today" and the Deleted list only. Same at lines 93-95, "a comment still present today on the rows_tx field in peek_stash.rs", a field that does not exist at 3cd9c996a3. The document deliberately sits on top of the stack so it can be read against the implemented tree, so every unqualified present tense has to be true of that tree.

Minor

• Lines 619-620 misquote compute: add configurable peek row iteration limit #38158. Its body says "the restart makes that count charge the same rows twice"; the document quotes "and the restart then charges the same rows twice", inside quotation marks.
• Lines 147 and 212-213 say take_batch is called "after each slice". Only in the Suspended arm (peek_offload.rs:310-314); a Complete slice carries its rows in the outcome.
• Lines 367-369: 10000 positions at 1 us is 10 ms, not "single-digit milliseconds". dyncfgs.rs:711-712 repeats it.
• The mermaid diagram shows two of walk's six exits, dropping Failed, UploadDemand::Satisfied, NO_STASH_LOCATION and the shard-open failure. It also draws the permit queue outside the task, whereas the task is spawned first and waits on the semaphore inside itself, which is why state 3's exit is a result_tx.closed() select rather than a queue departure.
• State 5 names discard for the abort path where it is impl Drop for StashUpload (peek_stash.rs:341-346), contradicting state 4, which says it correctly. It also never mentions that finish_batch hands the builder to a detached task and wraps the result in DeliveredBatch, whose drop deletes the batch specifically to cover a cancellation landing in the flush. That is the largest leak window the stack actually closes, and it appears in neither the Added list nor state 5. The caught panic at peek_stash.rs:315-336 is also live behaviour, not just the flush cost PER-70 tracks.
• Six measurements at lines 24-41 have no stated provenance: 5783.7 ms, 180.4 ms, 58 of 261, 5720.4 ms, 110.7 ms and 11.6 ms. No harness, dashboard or ticket is named, and nothing in the repo reproduces any of it; the open-loop driver they came from is an untracked file. Name the harness and where the raw series live, or label them as one-off staging observations.
• Line 355 asserts the inline budget is "Sized for point lookups, not for scans" while lines 625-626 concede 1024 "is a starting estimate, not a measurement". Line 316-318's "It is also mostly empty when it matters" is likewise unmeasured. Keep the honest version.
• "Seven layers, bottom to top" is already stale: compute: fuel PeekResultIterator's scan loop #38505 and compute: add configurable peek row iteration limit #38506 are merged.

Sections that check out

All five new dyncfgs by name, default and scope, and the three-against-two scope split with its rationale. The dyncfg arithmetic, seven added against two removed, net +5, verified by diffing dyncfgs.rs across the whole stack, including that PEEK_STASH_BATCH_SIZE did count rows. Every metric named exists with the described semantics and the substrate labels are literally inline and offloaded. Every other named symbol exists and behaves as described: ScanOutcome, take_batch, RowBatch, NO_STASH_LOCATION, OffloadedPeek::_abort_handle, StashUpload::push and discard, UploadDemand::Satisfied, InlineBudget with arm-on-first-grant, the three .max(1) floors, PeekStatus::Promote, RowCollection::new, TraceStorage<Tr> = Vec<Tr::Batch>, the fuel-carrying Literals with a non-seeking new, the uuid resume-point rotation with request_peek_activation, and the soft-panic-on-dead-task arm. The permit design, including the semaphore being built per serve call and the two-runtime over-admission being recorded there. persist_blob_target_size at 128 MiB, and the per-chunk max_result_size claim. All three referenced PRs confirmed open, with #38040's stat, yielding.rs, Budget::CLOCK_INTERVAL = 1024 and the unbudgeted-seek NOTE all verifying, and #38429's snapshot_for_offload, OffloadSnapshot, IndexOffloadPeek::_in_flight, spawn_blocking and inflight cap all verifying on its branch. Layers 2 through 7 match their commit ranges. Repo doc conventions are clean.

[antiguru]

Findings addressed, posted by Claude Code on behalf of @antiguru.

Every wrong claim from the fact-check is corrected, and the document is updated for what the stack does after this round of review.

Corrected. The quotation attributed to the code that exists nowhere in the repo. The fifth cancellation state, which is not a state: a cancellation landing mid inline slice arrives in one of the other four, because the worker inserts the promoted peek and returns within the same turn while handle_cancel_peek runs on the next command drain. The retention bound, which holds only for a scan that may use the stash. Content of the merged fueled-iterator layer that was never in it. "Fewer PendingPeek states than today", where the count is the same. A deleted spare cursor belonging to an unmerged proposal rather than to this stack. The deleted user-facing metric the Deleted list omitted. The cursor-setup timer, which is one measurement taken on the worker rather than a sum of slices across substrates. And the present-tense descriptions of code the layers below have already replaced.

Updated for the stack. All five parameters are replica-scoped, so the argument for pinning three of them environment-wide is gone. The permit bound is a nano-fraction of a runtime's workers rather than a count with a sentinel, and the section says how far one semaphore reaches and what a saturated bound does to the runtime's worker threads. A peek waiting on its turn keeps the worker from parking rather than asking timely to wake it. Fuel is charged only for positions a walk inspects, and the charge is independent of how the walk is sliced. The result-size ceiling bounds a stashed answer across the workers that produced it. The stash transition ships default-on and is outside the kill switch's rollback, which Kill switch now states.

Provenance. The measurements name where they came from and say they are one-off staging observations from a driver not in the repository, rather than reading as reproducible numbers. The diagram shows the exits a walk actually has instead of two of them.