Skip to content

feat: streaming Execution primitive, with run() as its wrapper (#683) - #691

Merged
iheitlager merged 4 commits into
mainfrom
feat/683-streaming-execution
Sep 7, 2026
Merged

feat: streaming Execution primitive, with run() as its wrapper (#683)#691
iheitlager merged 4 commits into
mainfrom
feat/683-streaming-execution

Conversation

@dpsiderius

@dpsiderius dpsiderius commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Every execution entry point (execute_with_db,
execute_with_db_and_params, execute_transaction_step) 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. It was also not fixable
from outside the crate: fn dispatch (src/vdbe/exec.rs:704) and fn run are
private, and ResultRow hands its row to Vm::emit_row, which pushes into a
Vec owned by the Vm.

Spike 014 (#682) measured the cost on a 1,000,000-row result:

batch streaming
peak heap 137.7 MB 8.68 MB
time to first row 5.36 ms 44.7 µs

This adds pub struct Execution<'p> (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 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 pending FIFO is not an accident

pending is a VecDeque drained from the front, not a pop off the back,
because pragma::integrity_check (src/vdbe/pragma.rs:110) emits one row per
problem 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.rs empties three indexes (writing a
valid-but-empty index leaf, page[0] = 0x0A) to force a genuinely multi-row
dispatch. 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

Notes for a reviewer

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 ResultRow emit loop, which OOMs the fuzz
target rather than reporting a finding. The drain is now bounded and a
result_row_emit_loop seed pins the case. Touches only tests/fuzz/.

Test plan

  • tests/unit/vdbe_streaming_execution_test.rs — 6 new tests: row/order
    equivalence with run for a scan, a LIMIT, an aggregate and an empty
    result; multi-row-single-dispatch order; errors and halts are terminal
    (a caller that keeps polling gets None, not a re-entered program)
  • cargo test --locked1568 passed / 0 failed (origin/main
    measured 1562 the same way: +6, exactly this ticket's tests)
  • cargo test --locked --test corpus — 380, unchanged from baseline
  • FIFO guard mutation-checked (pop-off-back → exactly one test fails)
  • make lint, cargo fmt --check, make check-mod-files clean
  • make assurance — 86/86, 276/276, no dead links
  • make lintboth clippy passes, including the second one naming
    the test = false corpus/parity/sqllogictest targets that --tests
    does not build. cargo clippy --all-targets does not cover them, and
    is the check I originally used
  • All of the above re-verified at 77d76e9 (branch tip, including the
    fuzz commit) from a clean detached worktree, not the working tree

Spend: matched the small estimate. The design was already paid for by
spike 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

dpsiderius and others added 3 commits September 1, 2026 17:08
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>
@iheitlager
iheitlager merged commit fc5ae4f into main Sep 7, 2026
10 checks passed
@iheitlager
iheitlager deleted the feat/683-streaming-execution branch September 7, 2026 16:30
dpsiderius added a commit that referenced this pull request Sep 7, 2026
#691's streaming `Execution` landed on `main` and moved the run loop out
of `fn run` and into `Execution::next_row`, which is where this branch
was reading `vm.changes` from. Resolved as #692's own issue text called
for ("the `Option<u64>` on the execution entry points, plus `Execution`
once #683 lands"): `Execution` grows a `changes()` accessor beside
`autocommit()`, and `run` — now a wrapper over `next_row` — carries the
count out as its third tuple element. `StepOutcome` and
`execute_transaction_step_counted` are unchanged.

Also in this merge:

- `tests/unit/vdbe_changes_test.rs::streaming_execution_reports_the_rows_it_changed`
  covers the new accessor. It pins absolute counts rather than
  streaming-vs-batch parity: `run` reads the count through this very
  accessor, so a parity test stays green when the accessor is stubbed to
  zero. Mutation-checked both ways.
- `Execution::changes`'s doc says "so far" and means it, since a caller
  that abandons a stream sees a partial count. It does not claim
  `RETURNING` as the motivating case — that is a V9 rule the tokenizer
  knows and the parser does not.

`Cargo.toml` and `.openspec/adr/index.md` were additive conflicts (both
sides appended an entry); both entries kept.

Gates on the merged tree: 1576 passed / 0 failed, corpus 388, `make lint`
clean, assurance 86/86 and 276/276 with no dead links introduced — the 24
it reports are #693's spec-013 `(planned)` links, identical on `main`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: streaming Execution primitive — read a result row without materializing the rest

2 participants