Export bounded grouped SQL inserts for local DB dumps - #2893
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: rainlanguage/raindex/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: rainlanguage/raindex/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSQL export now groups row tuples into multi-row ChangesSQL Export
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Merge Risk: ⚪ Minimal · up to No identified issue prevents merging after the normal checks and stack requirements are satisfied. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The grouped inserts remain one statement per line within the existing dump transaction. No new exposed operation or access-control change is evident, though interruption and concurrency behavior is not fully tested. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
How to use the Graphite Merge QueueAdd the label Raindex-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
| let by_bytes = build_insert_statements_bounded("items", &columns, &rows, 256, 75) | ||
| .expect("bound statement bytes"); | ||
| assert!(by_bytes.lines().count() > 1); | ||
| assert!(by_bytes.lines().all(|line| line.len() < 75)); | ||
|
|
||
| for sql in [by_rows, by_bytes] { | ||
| let conn = Connection::open_in_memory().expect("open database"); | ||
| conn.execute_batch("CREATE TABLE items (value TEXT NOT NULL);") | ||
| .expect("create table"); | ||
| conn.execute_batch(&sql).expect("import grouped inserts"); |
There was a problem hiding this comment.
minor: No test covers the two behaviors the description highlights. With max_bytes = 75, no single row is above the limit, so the oversized-row path is never taken. The flush happens at 84 > 75, far from the edge. If you change > to >=, remove the + 2, or remove the statement_rows > 0 guard (which would write a bare ; line), the tests still pass. Also, execute_batch(&sql) parses statements across lines, so it does not test the real importer (value.lines() into one statement per line in pipeline/engine.rs). Add one row that is larger than max_bytes and one tuple that lands exactly on max_bytes. Then import with sql.lines(), one statement per line, as the engine does.
| let values_sql = format_row_values(row, &column_names).map_err(LocalDbError::from)?; | ||
| output.push_str(&format!( | ||
| "INSERT INTO \"{table}\" ({quoted_columns}) VALUES ({values_sql});\n" | ||
| )); | ||
| let tuple = format!("({values_sql})"); |
There was a problem hiding this comment.
minor: This problem existed before this PR, but it affects the line-importer question you asked about. format_sql_value only doubles ', so a TEXT value that contains \n puts the statement on two lines. erc20_tokens.name/symbol come from on-chain name()/symbol() without sanitising, so anyone can deploy a token with a newline in its name. value.lines() in pipeline/engine.rs then gives two broken fragments, and the whole bootstrap transaction rolls back. Grouping does not make this worse, because one bad row already failed the import. It is fine as a follow-up: emit such literals with char(10)/replace(...) or as a hex cast, or split statements in the importer in a way that knows about quotes.
| table: &str, | ||
| columns: &[TableInfoRow], | ||
| rows: &[Value], | ||
| ) -> Result<String, LocalDbError> { | ||
| build_insert_statements_bounded(table, columns, rows, MAX_INSERT_ROWS, MAX_INSERT_BYTES) | ||
| } | ||
|
|
||
| fn build_insert_statements_bounded( | ||
| table: &str, | ||
| columns: &[TableInfoRow], | ||
| rows: &[Value], | ||
| max_rows: usize, | ||
| max_bytes: usize, | ||
| ) -> Result<String, LocalDbError> { |
There was a problem hiding this comment.
nit: build_insert_statements only forwards the two constants to build_insert_statements_bounded. One build_insert_statements(table, columns, rows, max_rows, max_bytes), with the constants passed at the call site on line 80 and in the default-limit test, removes the wrapper and the second name.
Merge activity
|
## Chained PRs - Depends on #2887. ## Motivation Filtered local DB dumps still contain one `INSERT` per row. That creates a large number of Rust SQL statement objects during browser bootstrap and adds avoidable parser work. Part of [RAI-2649](https://linear.app/makeitrain/issue/RAI-2649/produce-bounded-grouped-sql-dumps-and-versioned-browser-manifests). ## Solution - Group consecutive rows from the same table into multi-row `INSERT` statements, with at most 256 rows or 256 KiB per statement. A single row above the byte limit remains a standalone statement so the exporter preserves it. - Keep each statement on one line inside the existing single `BEGIN`/`COMMIT` dump transaction, so current line-based dump importers can read it. - Add tests for row and byte limits, SQL escaping, and round-trip imports into SQLite. This PR does not change the manifest schema, browser importer, or sqlite-web. The bounded atomic browser import API is tracked in [sqlite-web#35](rainlanguage/sqlite-web#35); wiring it into raindex remains a separate follow-up. ## Checks - `nix develop .#rust-shell --offline -c cargo fmt --all -- --check` — passed. - `nix develop .#rust-shell --offline -c cargo clippy --workspace --all-targets -- -D warnings` — passed. - `nix develop .#rust-shell --offline -c cargo test --workspace -- --test-threads=2` — passed. The default parallel run timed out in unrelated local Anvil fixture tests; limiting concurrency resolved it. - CI-equivalent `nix develop .#wasm-shell --offline` Wasm test command — passed; this host reported no runnable Wasm tests. - Three read-only simplification passes, two read-only local Codex reviews, and a CodeRabbit review — no actionable findings. Review focus: confirm multi-row `VALUES` remains compatible with the current one-statement-per-line dump importer. Unrelated unstaged benchmark experiments in the workspace are excluded from this PR.
66adc15 to
1e8019b
Compare
a018a4b to
984785c
Compare
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
## Related PRs and issue - [RAI-2650](https://linear.app/makeitrain/issue/RAI-2650/add-bounded-atomic-sql-dump-import-to-sqlite-web-and-release-it) - Producer grouped SQL change: [raindex#2893](rainlanguage/raindex#2893). This PR adds an upstream import API for the subsequent browser integration; neither PR needs the other to merge. - Builds on the existing `transaction(statements)` API from [sqlite-web#28](#28). ## Motivation The browser bootstrap currently materializes Rust and JavaScript statement objects for the SQL dump and sends the array through one transaction callback. Grouping rows on the producer side reduces this work, but the browser still needs a bounded way to stream SQL text into one atomic import without exposing partial data across tabs. ## Solution - Add `beginSqlDumpImport`, `appendSqlDumpChunk`, `finishSqlDumpImport`, and `cancelSqlDumpImport` to the public wasm API. The database worker owns one `BEGIN IMMEDIATE` transaction across chunks and commits only after a successful finish; SQL errors, invalid chunks, cancellation, and abandoned sessions roll back. - Parse SQL incrementally across chunk boundaries, including strings, comments, and trigger bodies. Bound each UTF-8 chunk to 512 KiB and an unfinished statement to 16 MiB. Reject row-returning statements during import to avoid collecting unbounded results. - Reject connection PRAGMA settings, `EXPLAIN`, and `ATTACH`/`DETACH` before preparation so failures cannot leave connection changes that transaction rollback would not undo. Accept and skip the standard `PRAGMA foreign_keys=OFF;`/`=0` dump header while preserving existing foreign-key enforcement. - Reject unpaired UTF-16 surrogates before conversion or dispatch. A cached JavaScript regex validates each chunk with one call across the Wasm boundary. - Run all core import tests against SQLite's memory VFS without silently skipping failed opens, alongside real OPFS browser integration. Cover partial lexical states, triggers, statement limits, rollback, cancellation, and caller-visible leader-loss outcomes. - Route import actions through the existing leader/follower coordinator. Reject competing queries and transactions while an import is active, limit outstanding import work per client instance, and expire abandoned imports after 120 seconds of inactivity on the next request or 30-second watchdog tick. An import response is reported only after the database worker resolves it, so a delayed commit cannot be reported as a follower timeout. - Document the API and limits; add browser integration tests and an isolated synthetic benchmark. This PR does not change the producer, the browser bootstrap call site, the dump format, or the published package version. The repository's main-branch release workflow publishes the next patch version after merge. ## Checks - [x] `nix develop -c build-submodules` - [x] `nix develop -c local-bundle` - [x] `nix develop -c rainix-rs-static` - [x] Core WASM tests in Chrome: 124 passed (15 core import tests execute through the memory VFS) - [x] Public WASM tests in Chrome: 42 passed - [x] `nix develop -c npm test` in `svelte-test`: 170 passed, 4 existing skips - [x] `nix develop -c npm run lint-format-check` in `svelte-test` - [x] `git diff --check` The latest WASM suites passed under the standard `nix develop -c test-wasm` wrapper with a matched Chrome for Testing / ChromeDriver 147 pair configured locally. The initial run with the system ChromeDriver 144 failed before tests started; the matched pair passed all tests. The packaged browser integration suite passed under Playwright Chromium. `nix develop -c npm run test:benchmark` also passed. Its three sequential cases import 10,000 equal rows: single-row transaction including construction 59.6 ms, grouped transaction 12.4 ms, and identical grouped SQL through import chunks 15.9 ms. This supersedes the earlier two-case comparison: grouping and the import API must be measured separately. This is a fixed-order smoke measurement with warm storage and small chunks, not an end-to-end or production speedup estimate; download, large-dump object allocation, and indexes require raindex benchmarks. Review focus: cross-tab serialization, transaction-marker handling, containment of connection state, and rollback behavior when a chunk or finish fails. A leader change before an import response reports an unknown outcome to the caller. Worker failure during finish can leave the outcome unknown even when an error response arrives; check/reset before retrying. Any lost finish response requires checking/resetting the database before retrying; the new test deliberately stalls a DB-worker response to make that path deterministic. Local Codex review completed three initial rounds with four reviewers, followed by two follow-up rounds with three reviewers, with no OpenCode supplement. Three simplification passes checked the follow-up fixes. All seven latest review comments are addressed: skipped headers are excluded from counts, empty dumps are consistently rejected, unmatched markers have a specific error, the unused queue guard is removed, benchmark APIs use identical grouped SQL, and the supported dumps and liveness/unknown-outcome limitations are documented. The earlier fixes also close the `EXPLAIN PRAGMA` bypass and prevent attachment changes from surviving cancellation. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for importing SQL dumps in chunks, with controls to begin, finish, or cancel an import across connected clients. * Imports validate chunk encoding and SQL statements, reject unsupported operations, and roll back on errors or after 120 seconds of inactivity. Regular database operations are blocked while an import is active. * **Documentation** * Added guidance on streaming SQL dumps, import constraints, failure handling, and checking the database before retrying when the commit outcome is unknown. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Motivation The remote producer still pins raindex before the unused-row and grouped-insert changes. Browser downloads therefore continue to contain raw RPC logs and signed take-order context rows, with one INSERT per retained row. Part of [RAI-2652](https://linear.app/makeitrain/issue/RAI-2652/update-remote-db-producer-for-optimized-browser-artifacts). ## Solution - Update `lib/raindex` from `6a78f29134f3125800ebae46db738ecb19077c69` to merged raindex main commit `984785c8a620072f515dc070fec0e732c9bfd156`. - Include [raindex#2887](rainlanguage/raindex#2887), which stops new raw/context writes and omits `raw_events`, `take_order_contexts`, and `context_values` from exports, including rows inherited from older dumps. - Include [raindex#2893](rainlanguage/raindex#2893), which groups retained INSERT rows with limits of 256 rows or 256 KiB per statement; oversized individual rows remain standalone. The SQL dump filename, gzip encoding, manifest schema, database schema, and publication flow remain compatible. The current browser transaction importer already supports these multi-row INSERT statements. This producer update does not depend on sqlite-web#35 or enable deferred browser indexes. ## Rollout After merging, dispatch the existing deploy workflow for the current deployment ID and settings URL. It rebuilds and installs the CLI from this pinned submodule, then starts the producer. The first successful run downloads the existing published dump, imports it into a temporary database, indexes newer blocks, and exports a filtered, grouped replacement. The runner uploads the dumps before publishing the manifest. No separate filtering script, empty manifest, or full historical reindex is needed. Verify each target's published dump omits the three unused tables, contains grouped INSERTs, and retains its watermark. Verify a fresh browser import and order/vault queries. Failed targets can retain their previous manifest entries and old dumps. Keep the previous manifest/dump objects available before rollout for rollback. ## Checks - [x] Confirm the pinned revision includes both upstream changes. - [x] `git diff --check`. - [x] `bash -n prep.sh nixos/local-db-remote-run.sh`. - [x] `nix run .#build-raindex-cli` — release CLI build passed on aarch64 Darwin with Rust/Cargo 1.94. - [x] CLI `--help` and `local-db sync --help` smoke checks. - [ ] Live deployment, publication, and fresh-browser verification; this PR does not execute them. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **User-facing changes** * No user-visible changes are described in the available summary. Any impact of this update is unclear, so no feature or behavior changes are listed. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Chained PRs
Motivation
Filtered local DB dumps still contain one
INSERTper row. That creates a large number of Rust SQL statement objects during browser bootstrap and adds avoidable parser work.Part of RAI-2649.
Solution
INSERTstatements, with at most 256 rows or 256 KiB per statement. A single row above the byte limit remains a standalone statement so the exporter preserves it.BEGIN/COMMITdump transaction, so current line-based dump importers can read it.This PR does not change the manifest schema, browser importer, or sqlite-web. The bounded atomic browser import API is tracked in sqlite-web#35; wiring it into raindex remains a separate follow-up.
Checks
nix develop .#rust-shell --offline -c cargo fmt --all -- --check— passed.nix develop .#rust-shell --offline -c cargo clippy --workspace --all-targets -- -D warnings— passed.nix develop .#rust-shell --offline -c cargo test --workspace -- --test-threads=2— passed. The default parallel run timed out in unrelated local Anvil fixture tests; limiting concurrency resolved it.nix develop .#wasm-shell --offlineWasm test command — passed; this host reported no runnable Wasm tests.Review focus: confirm multi-row
VALUESremains compatible with the current one-statement-per-line dump importer. Unrelated unstaged benchmark experiments in the workspace are excluded from this PR.