feat: streaming Execution primitive, with run() as its wrapper (#683) - #691
Open
dpsiderius wants to merge 3 commits into
Open
feat: streaming Execution primitive, with run() as its wrapper (#683)#691dpsiderius wants to merge 3 commits into
dpsiderius wants to merge 3 commits into
Conversation
Every execution entry point materialized a whole `Vec<Vec<Value>>` before returning, so reading row 0 of a large result cost building row N and a caller could not stop early. Spike 014 (#682) measured that at 137.7 MB peak heap and 5.36 ms to first row for a 1,000,000-row result, against 8.68 MB and 44.7 us streaming. It was also not fixable from outside the crate: `fn dispatch` and `fn run` are private, and `ResultRow` hands its row to `Vm::emit_row`, which pushes into a `Vec` inside the `Vm`. Adds `Execution` (`new`/`next_row`/`autocommit`) and reimplements `run()` as a wrapper that collects `next_row` into the same `Vec` it already returned. The wrapper is the load-bearing part: batch and streaming become literally the same loop, so they cannot drift, and the existing suite is the equivalence proof — 1562 passed / 0 failed before, 1568 / 0 after, the delta being exactly this ticket's six new tests. `pending` is a FIFO rather than a pop off the back because `pragma::integrity_check` emits one row per problem from a single dispatch, and draining from the back would silently reverse that output. No test in the suite reached that path before now, so `vdbe_streaming_execution_test.rs` empties three indexes to force a genuinely multi-row result — and the guard was mutation-checked: with the FIFO replaced by a pop, that test and only that test fails. Deliberately not included: any transaction-aware streaming constructor (`Vm::autocommit` is private, and read-only streaming is what Req 7 targets), and chunking, which #682 showed is a transport concern for the facade rather than the primitive. ADR-0038 records both, plus why a second parallel execution path was rejected. No CHANGELOG entry or version bump: this repo folds those into a separate `chore/*-fold-into-0.18.x` PR (e.g. #671), and ticket PRs do not carry them. #683's acceptance criteria said otherwise and were wrong about the convention. Refs: 013/Req-7, #683, #682, #678 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ecution # Conflicts: # .openspec/adr/index.md
…gate `vdbe_exec` feeds arbitrary bytecode to `execute()`, which hands back every row the program emitted. The decoder is free to build `ResultRow` with a register range up to `MAX_REGISTERS` and an `Init` whose negative P2 lands back on it (`to_pc` clamps to 0), so the result set grows without bound — 4.3 GB in ~1400 rows of 131,072 NULLs each, tripping libFuzzer's 4096 MB limit (CI run 33857317005). Not a regression from this branch. Replaying the artifact against `origin/main` grows at the same ~1 GB/s, because nothing drains `Vm::rows` there either; the streaming rewrite moves rows through a FIFO but `run()` still collects them all. The fuzzer only reached it now because its writable corpus is gitignored, so every CI run re-explores from the three committed seeds on a fresh random seed. Bounding accumulation in the engine would be the wrong fix — a `SELECT` that legitimately returns N rows must be allowed to return N rows, and real programs come from codegen, never from a caller handing the VM raw bytecode. So the bound goes in the harness: rows are pulled through `Execution` and dropped as they arrive, and `MAX_ROWS` caps the emitted work per input so a wide `ResultRow` cannot exhaust the budget in time instead of memory. Per ADR-0040 `run()` is this same loop plus a `Vec::push`, so the dispatch coverage that spec 009's no-panic-totality obligation (#89) is actually about is unchanged. The OOM input is committed as seed `result_row_emit_loop` per tests/fuzz/seeds/README.md, so the regression cannot come back unnoticed. Verified: the seed goes from SIGKILL to 175 ms; `make fuzz-smoke` is green across all seven targets; a 90s `vdbe_exec` run peaks at 360 MB. Throughput is unaffected by the cap (MAX_ROWS 8 vs 64 measured within noise), so 64 is kept for multi-row coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 4, 2026
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
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.
Summary
Every execution entry point (
execute_with_db,execute_with_db_and_params,execute_transaction_step) materialized a wholeVec<Vec<Value>>before returning, so reading row 0 of a large result costbuilding row N, and a caller could not stop early. It was also not fixable
from outside the crate:
fn dispatch(src/vdbe/exec.rs:704) andfn runareprivate, and
ResultRowhands its row toVm::emit_row, which pushes into aVecowned by theVm.Spike 014 (#682) measured the cost on a 1,000,000-row result:
This adds
pub struct Execution<'p>(new/next_row/autocommit) andreimplements
run()as a wrapper that collectsnext_rowinto the sameVecit already returned.The wrapper is the load-bearing design decision, not a convenience. Batch and
streaming become literally the same loop, so they cannot drift, and the
existing suite becomes the equivalence proof: 1562 passed / 0 failed before,
1568 / 0 after, the delta being exactly this ticket's six new tests. The
alternative — a second, parallel execution path — is what ADR-0040 rejects.
The
pendingFIFO is not an accidentpendingis aVecDequedrained from the front, not a pop off the back,because
pragma::integrity_check(src/vdbe/pragma.rs:110) emits one row perproblem from a single dispatch. Draining from the back would silently
reverse that output.
No test in the suite reached that path, because no existing test produces
more than one problem row — so this PR builds one:
vdbe_streaming_execution_test.rsempties three indexes (writing avalid-but-empty index leaf,
page[0] = 0x0A) to force a genuinely multi-rowdispatch. Mutation-checked: replacing the FIFO with a pop fails that test, and
only that test. Without it the guard would have been free to regress.
Deliberately not included
Vm::autocommitisprivate, and read-only streaming is what spec 013/Req 7 targets.
row and found chunking at ~1024 erases it, but
Executionhas no channel —that is a facade concern. Recorded in ADR-0040 so the facade ticket does not
rediscover it.
ValueSend— that is feat: make Value Send by switching Text/Blob payloads from Rc to Arc #688, in review alongside this.Notes for a reviewer
0040-streaming-execution-with-batch-as-wrapper.md, but the commit messagesays ADR-0038 — it was written before feat: private crate registry via JFrog Artifactory (#12) #684 landed and took 0038 for the
Cargo registry decision. The code citation (
exec.rs) says 0040 and iscorrect; the commit message is stale and I have left it rather than
force-push over a pushed branch. Merge feat: make Value Send by switching Text/Blob payloads from Rc to Arc #688 (ADR-0039) before this one
or the ADR sequence gains a gap.
make assurancedoes not move on this PR — still 86/86 and 276/276.Spec 013 is the subject of Feat/embedding api spec #678, which is still
Status: Proposedandunmerged, so Req 7 is not yet in the dashboard's denominator. This PR
implements the engine half of Req 7 ahead of the spec landing; the
dashboard should move when Feat/embedding api spec #678 does.
that is worth settling in that PR rather than here. It asks that "peak
allocation is proportional to the ten rows"; the measured streaming floor is
8.68 MB, dominated by the page cache and flat in result size. spike: 014 embedding-API kernel — Send+Sync handle over a streaming VDBE #682 found
the whole floor is one constant —
DEFAULT_PAGE_CACHE_CAPACITY(
pager.rs:63): 2000 pages → 8.68 MB, 256 → 1.10 MB, 64 → 291 KB (445xbetter than batch), at a cost of ~4.5% on streaming throughput and nothing
on batch. The provable claim is "peak allocation is independent of result
size", which is the stronger property anyway. Filed as a follow-up on
Feat/embedding api spec #678, not fixed here.
CHANGELOG.mdentry or version bump: this repo folds those into aseparate
chore/*-fold-into-0.18.xPR (precedent chore: fold #663 into 0.18.9, revert premature 0.18.10 bump #671). feat: streaming Execution primitive — read a result row without materializing the rest #683's ownacceptance criteria ask for a CHANGELOG entry and are wrong about the
convention — I wrote them; the ticket needs correcting, not this PR.
A second commit, from the fuzz gate
fix(fuzz): bound vdbe_exec's row drain so an emit-loop can't OOM the gate(+37/−4, plus one seed) is a direct consequence of this change rather than
unrelated housekeeping. Making a row drain reachable means a fuzz-generated
program can drive an unbounded
ResultRowemit loop, which OOMs the fuzztarget rather than reporting a finding. The drain is now bounded and a
result_row_emit_loopseed pins the case. Touches onlytests/fuzz/.Test plan
tests/unit/vdbe_streaming_execution_test.rs— 6 new tests: row/orderequivalence with
runfor a scan, aLIMIT, an aggregate and an emptyresult; multi-row-single-dispatch order; errors and halts are terminal
(a caller that keeps polling gets
None, not a re-entered program)cargo test --locked— 1568 passed / 0 failed (origin/mainmeasured 1562 the same way: +6, exactly this ticket's tests)
cargo test --locked --test corpus— 380, unchanged from baselinemake lint,cargo fmt --check,make check-mod-filescleanmake assurance— 86/86, 276/276, no dead linksmake lint— both clippy passes, including the second one namingthe
test = falsecorpus/parity/sqllogictest targets that--testsdoes not build.
cargo clippy --all-targetsdoes not cover them, andis the check I originally used
77d76e9(branch tip, including thefuzz commit) from a clean detached worktree, not the working tree
Spend: matched the
smallestimate. The design was already paid for byspike 014; what was not in the estimate was the multi-row-dispatch fixture,
since the suite had no way to produce one.
Refs: 013/Req-7, #682, #678, #688
Closes #683
🤖 Generated with Claude Code