Skip to content

perf: apply a per-table hook's rows at stdout EOF, before its exit is reaped - #1565

Merged
thekevinbot merged 5 commits into
mainfrom
bot/1562-hook-savepoint-1102
Nov 3, 2026
Merged

thekevinbot merged 5 commits into
mainfrom
bot/1562-hook-savepoint-1102

Conversation

@thekevinbot

@thekevinbot thekevinbot commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #1562

Root cause

On a watched change, a per-table on-file command's rows were applied only after wait() reaped the child. cat closes stdout before exit_group, so its exit teardown (about 21us) sat on the critical path between the last row and the event.

Change

  • command::drain_then_reap drains stdout in whole-line blocks, calls on_eof once stdout closes, and only then reaps the child.
  • refresh_table stages the rows at EOF inside a dirsql_unit SQLite savepoint (foreign_key::stage), while the child is still exiting. It keeps them (RELEASE) on a zero exit and discards them (ROLLBACK TO) on a nonzero exit or failure. The hook error event outranks everything else, as before.
  • A multi-invocation (pooled) run never ends early; it stages after the hook returns, as before.
  • Semantics are unchanged: a failed command leaves no rows, and the events emitted are the same. Hand-checked with dirsql server: cat produces one update event and the new row; a command that prints rows then exits 3 produces one error event and leaves the old row.

Measurements

main (05880bb) this branch
in-process apply_file_events per change, cat hook (median of 3000 iterations, 5 interleaved runs) 230-246us (about 238) 209-227us (about 216)
watch_speed_test passes (1.1x bar), 40 interleaved rounds 27/40 (68%) 39/40 (98%)
watch_speed_test median ratio, second batch of 25 rounds about 1.07 about 0.99

The host load average was about 15 on 32 cores. The rounds ran interleaved (main, then branch), with the two release CLIs on PATH.

Changelog / Migrations

  • Changelog fragment added under packages/rust/changelog.d/ (2026-11-02-stage-per-table-rows-at-eof.md)
  • Migration fragment with the same slug added under packages/rust/migrations.d/ (every subsection but Summary _None._)

E2E Verification

  • Ran e2e suites locally for every affected SDK
  • Python SDK e2e: pass
  • TypeScript SDK e2e: pass
  • Rust core e2e: pass (cargo test -p dirsql --features cli)
  • packages/python/e2e-attestations/<branch>.json receipt written
  • packages/ts/e2e-attestations/<branch>.json receipt written
  • Command(s) run: testing-conventions e2e attest 'uv run python -m pytest tests/e2e/{on_file_query,query_contract,declared_table_name,fanout,use_case_dossier,use_case_join,use_case_notebook,use_case_transcripts,watch_speed}_test.py -q' in packages/python (after maturin develop --release); just e2e-attest-ts
  • Result summary: Python 22 passed; TypeScript 18 passed.

@thekevinbot
thekevinbot marked this pull request as ready for review November 3, 2026 02:28
@thekevinbot
thekevinbot merged commit 271f1f3 into main Nov 3, 2026
89 checks passed
@thekevinbot
thekevinbot deleted the bot/1562-hook-savepoint-1102 branch November 3, 2026 12:28
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.

Python e2e watch_speed_test fails at its 1.1x bar on main

1 participant