From 4b11e628e9126229a78627482314c9dca0ff0b94 Mon Sep 17 00:00:00 2001 From: Ilja Heitlager Date: Sat, 29 Aug 2026 12:09:55 +0200 Subject: [PATCH] perf: cache WAL resume state on Pager to skip per-flush rescan (#640) WalWriter::open_existing re-read and rescanned the entire -wal file on every commit (ADR-0026's accepted trade-off), measured at ~6-7ms per commit even against a near-empty WAL (#635's profiling). Cache a small WalResumeHint (header, offset, running checksum, expected file size) on Pager, refreshed after each flush and invalidated at the same three sites the existing wal_shm handle cache (#437) already resets at (switch_wal_to_journal, switch_journal_to_wal, recreate_wal_locked). open_existing trusts the hint only when the file's actual size and a cheap header re-read both still match, so a concurrent writer, mode switch, or torn file always falls back to the original full rescan. Supersedes ADR-0026's rejected caching alternative with ADR-0035, since 0026 is cited by this ticket. spend: matched estimate (~Medium) Co-Authored-By: Claude Sonnet 5 --- ...5-wal-resume-hint-cache-supersedes-0026.md | 99 ++++++++++ .openspec/adr/index.md | 1 + src/pager.rs | 183 ++++++++++++++++-- src/pager/wal.rs | 83 +++++++- 4 files changed, 347 insertions(+), 19 deletions(-) create mode 100644 .openspec/adr/0035-wal-resume-hint-cache-supersedes-0026.md diff --git a/.openspec/adr/0035-wal-resume-hint-cache-supersedes-0026.md b/.openspec/adr/0035-wal-resume-hint-cache-supersedes-0026.md new file mode 100644 index 00000000..7ee44c54 --- /dev/null +++ b/.openspec/adr/0035-wal-resume-hint-cache-supersedes-0026.md @@ -0,0 +1,99 @@ +# 0035: `Pager`-cached WAL resume hint supersedes ADR-0026's per-flush rescan + +Date: 2026-08-29 + +Status: Accepted + +Supersedes: ADR-0026 ("Alternatives rejected" — caching a `WalWriter` +handle/resume state across flushes) + +## Context + +#640 (follow-up from #635's profiling) measured `WalWriter::open_existing`'s +full read-and-rescan of `-wal` on every commit at ~6-7ms even against a +near-empty, freshly-created WAL file — a meaningful fraction of the commit +path's total cost, and exactly the scenario ADR-0026's own "Consequences" +section predicted as the trigger for revisiting the rescan-per-flush +trade-off: *"will need revisiting — most likely by tracking the resume +offset/checksum on `Pager` across calls, invalidated on mode switches — if a +long-lived WAL under sustained write load makes per-commit rescanning +measurably slow."* + +ADR-0026 rejected caching a `WalWriter` handle on `Pager` "for now", citing +two problems that needed solving first: invalidating the cache on +`switch_wal_to_journal`/`switch_journal_to_wal` (which delete or recreate +`-wal`), and tolerating a torn/short file from a `Drop`-order crash or a +concurrent external writer between flushes. + +## Decision + +**Cache a small `WalResumeHint` (header, append offset, running checksum, +and the file size that state is only valid against) on `Pager`, not a whole +`WalWriter` handle.** `flush_wal_locked` hands its cached hint into +`WalWriter::open_existing`, which now takes an `Option<&WalResumeHint>`: + +- If the `-wal` file's current size matches `hint.expected_size` *and* a + cheap 32-byte re-read of the header matches `hint.header` byte-for-byte, + the hint is trusted and the full read-and-rescan is skipped entirely — + the writer resumes directly from the cached offset/running checksum. +- Any mismatch (a concurrent writer or checkpoint changed the file, a mode + switch replaced it, a crash left it torn) falls back to exactly the + full read + `last_valid_frame_state` rescan ADR-0026 already established, + so correctness never depends on the cache being right — only commit + latency does. + +`Pager::wal_resume: Option` is populated after every +successful `WalWriter::sync`, mirroring the `wal_shm` handle cache (#437) +already on `Pager`: lazily populated, and reset to `None` at the same three +sites `wal_shm` is reset at — `switch_wal_to_journal`, +`switch_journal_to_wal`, and `recreate_wal_locked` (the #422 "`-wal` +vanished out from under this connection" recovery path) — since each of +those deletes or recreates the underlying file, making any cached hint +stale by construction. + +This resolves ADR-0026's rejected-alternative concerns without needing a +`WalWriter` handle's own lifetime managed across calls: the hint is `Copy` +data, not a live file handle, so there is no fd to keep valid across a mode +switch, and the size+header check — not trust in the cache's own +invalidation completeness — is what makes a torn file or concurrent writer +safe to resume past. + +## Alternatives rejected + +- **Cache the whole `WalWriter` (ADR-0026's original rejected option)**: + still avoids managing an fd's lifetime across mode switches more cleanly + than a plain data hint would — a cached handle would need its own + re-validation logic duplicating what the hint's size/header check already + does, for no benefit over caching just the state the handle would + otherwise re-derive from a rescan anyway. +- **Trust the file size alone, skip the header re-read**: rejected — sizes + can coincide across a generation change this cache wasn't told about + (e.g. an external checkpoint truncates `-wal` back to header-only, then a + fresh writer's own frames happen to grow it back to the same total length + the old generation had), which would resume against the wrong + salts/checksum chain and silently corrupt the file. The extra 32-byte + read is O(1), not O(WAL size), so it costs nothing measurable while + closing that gap. +- **Track `mxFrame` from `-shm` instead of a `Pager`-local hint**: rejected + — `mxFrame` publication is a best-effort, non-atomic `pwrite` (ADR-0026's + own accepted residual risk), and a torn read of it would be a worse + invalidation signal than the on-disk `-wal` file's own actual size, which + is exactly what a writer is about to append onto regardless. + +## Consequences + +- A commit against an already-warm cache costs one `size()` stat plus one + 32-byte header read instead of reading and rescanning the entire `-wal` + file — the dominant cost #640 measured is gone for the common case of + consecutive commits from the same long-lived `Pager`. +- The full rescan path (and its cost) is unchanged and still exercised + automatically whenever the hint can't be trusted, so a first commit after + `Pager::open`, a mode switch, or a concurrent writer's interleaved commit + pays exactly what ADR-0026 always charged — no new failure mode, only a + narrower set of calls that pay it. +- `Pager` now carries one more `Copy` field (`wal_resume`) alongside + `wal_shm`, with the same three invalidation sites — a future change to + either cache's invalidation logic should double-check the other still + agrees, since they're deliberately kept in lockstep rather than merged + into one struct (the `-shm` handle is a live resource with its own + lifetime; the resume hint is plain data with none). diff --git a/.openspec/adr/index.md b/.openspec/adr/index.md index b5b59774..98cb39cf 100644 --- a/.openspec/adr/index.md +++ b/.openspec/adr/index.md @@ -38,3 +38,4 @@ Specs record what the system must do; ADRs record **why it is shaped this way** | [0032](0032-hash-group-by-second-strategy.md) | Hash `GROUP BY` is a second strategy with its own opcode family, and still emits groups in key order | 2026-08-27 | | [0033](0033-constant-propagation-and-or-to-in-extend-fast-paths-in-place.md) | Constant propagation and OR-to-IN extend existing equality fast paths in place; only genuine range seeks wait for a new opcode | 2026-08-28 | | [0034](0034-index-range-seeks.md) | Real-index range seeks for `BETWEEN`/`IN`/`LIKE`-prefix (`SeekIndexGE`/`IdxCompareGT`) | 2026-08-28 | +| [0035](0035-wal-resume-hint-cache-supersedes-0026.md) | `Pager`-cached WAL resume hint supersedes ADR-0026's per-flush rescan | 2026-08-29 | diff --git a/src/pager.rs b/src/pager.rs index ffcd27f0..b46a4bfb 100644 --- a/src/pager.rs +++ b/src/pager.rs @@ -270,6 +270,22 @@ pub struct Pager { /// the underlying `-shm` file is deleted (`switch_wal_to_journal`) or /// freshly recreated (`switch_journal_to_wal`) there. wal_shm: Option, + /// A cached [`wal::WalResumeHint`] (ADR-0027) letting + /// [`Pager::flush_wal_locked`] skip `WalWriter::open_existing`'s + /// read-and-rescan of the whole `-wal` file when it's still valid — + /// populated after every successful `WalWriter::sync` in this + /// connection's lifetime, `None` for a connection that hasn't + /// committed in WAL mode yet. Reset to `None` at the same three + /// sites `wal_shm` above is: `switch_wal_to_journal`, + /// `switch_journal_to_wal`, `recreate_wal_locked` — each deletes or + /// recreates the underlying `-wal` file, so a hint captured against + /// the old file must never be handed to a writer opened against the + /// new one. `open_existing` itself also falls back to a full rescan + /// whenever the file's actual size doesn't match the hint (a + /// concurrent external writer, or a torn file from a crash), so this + /// cache never needs to be "perfectly" invalidated — only cheaply + /// invalidated at the points where staleness is certain. + wal_resume: Option, source: WritablePageSource, /// Committed WAL overlay pages, shared as `Rc<[u8]>` so a read hit /// hands out a refcount bump instead of copying `page_size` bytes @@ -425,6 +441,7 @@ impl Pager { tx_lock_level: crate::vfs::LockLevel::Shared, wal_lock, wal_shm: None, + wal_resume: None, source, wal_pages, dirty: HashMap::new(), @@ -666,20 +683,22 @@ impl Pager { // checkpoint backfills them. let post_page_count = read_be_u32(&self.read_page(1)?, PAGE_COUNT_OFFSET)?; - let mut writer = - match wal::WalWriter::open_existing(&self.vfs, &wal_path, self.page_size) { - Ok(writer) => writer, - // The `-wal` this `Pager` believed was live has vanished — - // e.g. a concurrent `sqlite3` connection auto-checkpointed - // and deleted `-wal`/`-shm` on close (#422). `journal_mode` - // still says `Wal` (this closure only ever runs from that - // branch), so recover exactly as `switch_journal_to_wal` - // creates one from scratch, rather than failing the commit. - Err(wal::WalError::Vfs(VfsError::NotFound { .. })) => { - self.recreate_wal_locked()? - } - Err(source) => return Err(to_pager_error(source)), - }; + let mut writer = match wal::WalWriter::open_existing( + &self.vfs, + &wal_path, + self.page_size, + self.wal_resume.as_ref(), + ) { + Ok(writer) => writer, + // The `-wal` this `Pager` believed was live has vanished — + // e.g. a concurrent `sqlite3` connection auto-checkpointed + // and deleted `-wal`/`-shm` on close (#422). `journal_mode` + // still says `Wal` (this closure only ever runs from that + // branch), so recover exactly as `switch_journal_to_wal` + // creates one from scratch, rather than failing the commit. + Err(wal::WalError::Vfs(VfsError::NotFound { .. })) => self.recreate_wal_locked()?, + Err(source) => return Err(to_pager_error(source)), + }; let last_index = page_nums.len().saturating_sub(1); for (index, &page_num) in page_nums.iter().enumerate() { @@ -695,6 +714,7 @@ impl Pager { } } writer.sync().map_err(to_pager_error)?; + self.wal_resume = Some(writer.resume_hint()); let new_mx_frame = writer.frame_count(); match &self.wal_shm { @@ -811,6 +831,13 @@ impl Pager { shm_file.sync()?; self.wal_shm = self.vfs.open_wal_shm(&self.db_path)?; + // The resume hint (ADR-0027), if any, described the now-deleted + // generation of `-wal`; `flush_wal_locked` overwrites this with + // the fresh writer's own hint once its commit succeeds, but + // clear it here too so a failure before that point never leaves + // a stale hint pointing at a generation this `Pager` just + // discarded. + self.wal_resume = None; Ok(writer) } @@ -846,8 +873,12 @@ impl Pager { // Drop any handle cached (#437) against a now-stale `-shm` // generation — `flush_wal_locked` reopens fresh against the file - // just created above on its next call. + // just created above on its next call. Likewise drop any cached + // resume hint (ADR-0027): it was captured against whatever + // generation of `-wal` existed before this call, which no longer + // exists. self.wal_shm = None; + self.wal_resume = None; Ok(()) } @@ -899,8 +930,11 @@ impl Pager { // The cached handle (#437), if any, points at the `-shm` file // just deleted above — drop it so a future switch back to WAL // reopens fresh rather than reusing a stale fd to a since- - // deleted (or reused-inode) file. + // deleted (or reused-inode) file. The cached resume hint + // (ADR-0027) is stale for the same reason: the `-wal` file it + // describes no longer exists. self.wal_shm = None; + self.wal_resume = None; Ok(()) } @@ -1808,6 +1842,123 @@ mod tests { assert_eq!(pages.get(&2), Some(&vec![9u8; 512])); } + /// ADR-0027: two consecutive commits from the same `Pager` must both + /// be correct once the second one resumes from the cached + /// `wal_resume` hint instead of rescanning the whole `-wal` file — + /// the checksum chain `wal::committed_pages` verifies on read would + /// break immediately if the cached offset/running-checksum state + /// were wrong. + #[test] + fn flush_wal_mode_second_commit_resumes_correctly_from_cached_hint() { + let mut vfs = MemoryVfs::new(); + let mut contents = vec![1u8; 512]; + write_be_u32(&mut contents, PAGE_COUNT_OFFSET, 2).unwrap(); + contents.extend(vec![2u8; 512]); + vfs.insert("/test.db", contents); + let mut pager = Pager::open(&vfs, Path::new("/test.db"), 512).unwrap(); + pager.set_journal_mode(JournalMode::Wal).unwrap(); + + pager.get_page_mut(2).unwrap().fill(9u8); + pager.flush().unwrap(); + assert!(pager.wal_resume.is_some()); + + pager.get_page_mut(2).unwrap().fill(11u8); + pager.flush().unwrap(); + + let wal_file = vfs.open_read(Path::new("/test.db-wal")).unwrap(); + let size = wal_file.size().unwrap(); + let mut wal_bytes = vec![0u8; size as usize]; + wal_file.read_at(&mut wal_bytes, 0).unwrap(); + let header = wal::WalHeader::parse(&wal_bytes).unwrap(); + let (pages, db_size) = wal::committed_pages(&header, &wal_bytes); + assert_eq!(db_size, 2); + assert_eq!(pages.get(&2), Some(&vec![11u8; 512])); + assert_eq!(pager.read_page(2).unwrap(), Rc::from(vec![11u8; 512])); + } + + /// ADR-0027: a mode round trip (WAL -> Legacy -> WAL) must invalidate + /// the cached `wal_resume` hint, since `switch_wal_to_journal` + /// deletes the old `-wal` file and `switch_journal_to_wal` creates an + /// unrelated one with fresh salts — resuming against the stale hint + /// would append onto (or validate against) a generation that no + /// longer exists. + #[test] + fn flush_wal_mode_after_mode_round_trip_does_not_reuse_stale_hint() { + let mut vfs = MemoryVfs::new(); + let mut contents = vec![1u8; 512]; + write_be_u32(&mut contents, PAGE_COUNT_OFFSET, 2).unwrap(); + contents.extend(vec![2u8; 512]); + vfs.insert("/test.db", contents); + let mut pager = Pager::open(&vfs, Path::new("/test.db"), 512).unwrap(); + pager.set_journal_mode(JournalMode::Wal).unwrap(); + + pager.get_page_mut(2).unwrap().fill(9u8); + pager.flush().unwrap(); + assert!(pager.wal_resume.is_some()); + + pager.set_journal_mode(JournalMode::Legacy).unwrap(); + assert!(pager.wal_resume.is_none()); + pager.set_journal_mode(JournalMode::Wal).unwrap(); + assert!(pager.wal_resume.is_none()); + + pager.get_page_mut(2).unwrap().fill(11u8); + pager.flush().unwrap(); + + let wal_file = vfs.open_read(Path::new("/test.db-wal")).unwrap(); + let size = wal_file.size().unwrap(); + let mut wal_bytes = vec![0u8; size as usize]; + wal_file.read_at(&mut wal_bytes, 0).unwrap(); + let header = wal::WalHeader::parse(&wal_bytes).unwrap(); + let (pages, db_size) = wal::committed_pages(&header, &wal_bytes); + assert_eq!(db_size, 2); + assert_eq!(pages.get(&2), Some(&vec![11u8; 512])); + } + + /// ADR-0027: a concurrent writer appending frames to `-wal` between + /// two commits from this `Pager` must be detected by the resume + /// hint's size check and force a full rescan, rather than resuming + /// the checksum chain from a stale cached offset and corrupting the + /// file — the same hazard ADR-0026 accepted the full-rescan cost to + /// avoid in the first place. + #[test] + fn flush_wal_mode_falls_back_to_rescan_when_wal_grew_from_elsewhere() { + let mut vfs = MemoryVfs::new(); + let mut contents = vec![1u8; 512]; + write_be_u32(&mut contents, PAGE_COUNT_OFFSET, 3).unwrap(); + contents.extend(vec![2u8; 512]); + contents.extend(vec![3u8; 512]); + vfs.insert("/test.db", contents); + + let mut writer_a = Pager::open(&vfs, Path::new("/test.db"), 512).unwrap(); + writer_a.set_journal_mode(JournalMode::Wal).unwrap(); + writer_a.get_page_mut(2).unwrap().fill(9u8); + writer_a.flush().unwrap(); + assert!(writer_a.wal_resume.is_some()); + + // A second connection, sharing the same underlying `-wal` file, + // commits a frame `writer_a` never learns about through its own + // cache. + let mut writer_b = Pager::open(&vfs, Path::new("/test.db"), 512).unwrap(); + writer_b.get_page_mut(3).unwrap().fill(7u8); + writer_b.flush().unwrap(); + + // `writer_a`'s cached hint still reflects the file as it was + // after its own commit, not `writer_b`'s — the size check inside + // `WalWriter::open_existing` must catch the mismatch and rescan. + writer_a.get_page_mut(2).unwrap().fill(11u8); + writer_a.flush().unwrap(); + + let wal_file = vfs.open_read(Path::new("/test.db-wal")).unwrap(); + let size = wal_file.size().unwrap(); + let mut wal_bytes = vec![0u8; size as usize]; + wal_file.read_at(&mut wal_bytes, 0).unwrap(); + let header = wal::WalHeader::parse(&wal_bytes).unwrap(); + let (pages, db_size) = wal::committed_pages(&header, &wal_bytes); + assert_eq!(db_size, 3); + assert_eq!(pages.get(&2), Some(&vec![11u8; 512])); + assert_eq!(pages.get(&3), Some(&vec![7u8; 512])); + } + /// #389's "readers don't block writers, writers don't block readers, /// reader sees a consistent snapshot" invariant: a `Pager` opened /// before a commit keeps its pre-commit view even after a second diff --git a/src/pager/wal.rs b/src/pager/wal.rs index 62604256..fc7f73a8 100644 --- a/src/pager/wal.rs +++ b/src/pager/wal.rs @@ -472,6 +472,20 @@ pub struct WalWriter { scratch: Vec, } +/// A `Pager`-cached snapshot of a [`WalWriter`]'s resume state (ADR-0027): +/// the header, append offset, and running checksum a previous flush left +/// off at, plus the file size that state is only valid against. Handed +/// back into [`WalWriter::open_existing`] so a commit that finds the +/// `-wal` file unchanged since the hint was captured can skip reading and +/// rescanning the whole file. +#[derive(Debug, Clone, Copy)] +pub struct WalResumeHint { + header: WalHeader, + offset: u64, + running: (u32, u32), + expected_size: u64, +} + impl WalWriter { /// Creates (or reopens) the `-wal` file at `path` and writes `header`. pub fn create(vfs: &AnyVfs, path: &Path, header: WalHeader) -> Result { @@ -553,9 +567,55 @@ impl WalWriter { /// `Err` otherwise), the same consistency check /// `crate::pager::read_wal_pages`/`checkpoint_passive` already apply /// when merging/checkpointing this same file. - pub fn open_existing(vfs: &AnyVfs, path: &Path, page_size: u32) -> Result { + /// + /// `resume_hint` (ADR-0027) short-circuits the read-and-rescan below: + /// when the file's actual size matches `hint.expected_size`, nothing + /// has appended to or truncated the file since the hint was captured + /// (this crate's own writer always leaves `offset == file len` after + /// `sync`, per [`WalWriter::frame_count`]'s doc comment), so the + /// hint's header/offset/running can be trusted as-is instead of + /// re-deriving them from a full read + [`last_valid_frame_state`] + /// walk. Any mismatch — a concurrent external writer, a mode switch, + /// a torn file from a crash — falls back to the full rescan exactly + /// as before the hint existed. + pub fn open_existing( + vfs: &AnyVfs, + path: &Path, + page_size: u32, + resume_hint: Option<&WalResumeHint>, + ) -> Result { let file = vfs.open_write(path)?; let size = file.size()?; + + if let Some(hint) = resume_hint { + // Two checks, not just the size: a same-size coincidence is + // possible across a generation change this cache wasn't told + // about (e.g. an external checkpoint truncates `-wal` back to + // just its header, then a fresh writer's frames happen to + // grow it back to the same total length the old generation + // had) — trusting size alone could resume against the wrong + // salts/checksum chain. Re-reading just the 32-byte header is + // O(1), not O(WAL size), so it's cheap insurance that the + // hint's `header` still matches the file currently on disk. + let mut header_bytes = [0u8; HEADER_LEN]; + let read_header = file + .read_at(&mut header_bytes, 0) + .ok() + .filter(|&n| n == HEADER_LEN) + .and_then(|_| WalHeader::parse(&header_bytes).ok()); + if hint.expected_size == size && read_header == Some(hint.header) { + return Ok(WalWriter { + file, + header: hint.header, + running: hint.running, + offset: hint.offset, + pending_offset: None, + pending: Vec::new(), + scratch: Vec::new(), + }); + } + } + let mut bytes = vec![0u8; size as usize]; let n = file.read_at(&mut bytes, 0)?; bytes.truncate(n); @@ -580,6 +640,23 @@ impl WalWriter { }) } + /// Snapshots this writer's resume state after a [`WalWriter::sync`] + /// (ADR-0027), for a caller (`Pager`) to cache across flushes and + /// hand back to the next [`WalWriter::open_existing`] call via + /// `resume_hint`. `expected_size` is `self.offset`: this writer's own + /// appends are the only thing that can have grown the file since it + /// was opened, and `sync` always writes `pending` up to exactly + /// `self.offset`, so the file's length and `self.offset` agree by + /// construction the moment `sync` returns. + pub fn resume_hint(&self) -> WalResumeHint { + WalResumeHint { + header: self.header, + offset: self.offset, + running: self.running, + expected_size: self.offset, + } + } + /// Total frames now in the WAL, including any written before this /// writer was opened (#389's `mxFrame`). `self.offset` is always /// `HEADER_LEN + frame_count * frame_size` by construction: `create` @@ -783,7 +860,7 @@ mod tests { } { - let mut writer = WalWriter::open_existing(&vfs, path, 512).unwrap(); + let mut writer = WalWriter::open_existing(&vfs, path, 512, None).unwrap(); assert_eq!( writer.frame_count(), 1, @@ -819,7 +896,7 @@ mod tests { let header = WalHeader::new(true, 512, 1, 2, 1); WalWriter::create(&vfs, path, header).unwrap(); - let result = WalWriter::open_existing(&vfs, path, 4096); + let result = WalWriter::open_existing(&vfs, path, 4096, None); assert!(matches!( result, Err(WalError::InvalidPageSize { page_size: 512 })