feat: add fetch/unfetch to Vfs trait (xFetch/xUnfetch, iVersion 3)#81
Open
russellromney wants to merge 4 commits intoorbitinghail:mainfrom
Open
feat: add fetch/unfetch to Vfs trait (xFetch/xUnfetch, iVersion 3)#81russellromney wants to merge 4 commits intoorbitinghail:mainfrom
russellromney wants to merge 4 commits intoorbitinghail:mainfrom
Conversation
Add fetch() and unfetch() methods to the Vfs trait with safe defaults: - fetch() returns Ok(None), telling SQLite to fall back to xRead - unfetch() is a no-op Wire x_fetch/x_unfetch C shims into io_methods, making iVersion=3 legitimate. VFS implementations that want mmap can override fetch() to return NonNull pointers to memory-mapped regions. 4 tests: - Basic write/read roundtrip with default fetch - Concurrent WAL (1W + 4R, 3 seconds) with default fetch (30/30) - Checkpoint under load exercises xFetch code path - Verify iVersion=3 works end-to-end
Author
|
simplifying tests. |
Author
|
Ready for review. |
carlsverre
reviewed
Apr 13, 2026
Contributor
carlsverre
left a comment
There was a problem hiding this comment.
The implementation looks good. Can you add fetch/unfetch support to your minimal vfs to exercise it? Make sure that something asserts they are actually reached in testing. I'm not convinced from reading the docs that they are. It sounds like mmap_size has to be set?
Author
|
Good point. Adding |
Implement actual mmap-based fetch() in test VFS. Assert that SQLite calls fetch() via atomic counter (PRAGMA mmap_size=1048576 required). Two tests: - test_fetch_mmap_reads: 200 rows, read back through mmap, assert fetch called - test_fetch_survives_checkpoint: 2500 rows triggers auto-checkpoint under mmap
Author
|
updated. test VFS now implements real mmap fetch/unfetch, and an atomic counter asserts that SQLite actually calls fetch() (requires PRAGMA mmap_size). ready for review. |
carlsverre
reviewed
Apr 15, 2026
Contributor
carlsverre
left a comment
There was a problem hiding this comment.
just some small tweaks then we can land this! thank you
Move fetch/unfetch counters from global statics into per-VFS Arc<FetchCounters>. Each test gets its own counters via setup(), safe for parallel test execution. Add unfetch assertion to test_fetch_mmap_reads (carlsverre review). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
All addressed, great feedback thx. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Follows from #79 and #80. PR #80 fixes the SEGFAULT by setting iVersion=2. This PR is the alternative: properly implement iVersion=3 by adding fetch/unfetch to the Vfs trait.
Changes
Add
fetch()andunfetch()methods to theVfstrait:Default implementations decline mmap (
fetchreturnsNone,unfetchis a no-op). SQLite gracefully falls back toxRead. VFS implementations that want memory-mapped page reads can overridefetch()to returnNonNullpointers.C shims
x_fetchandx_unfetchwired intoio_methods.iVersionstays at 3, now legitimately.Use case
We're building turbolite, a SQLite VFS that serves queries from S3 with a local NVMe cache. With
fetch(), we can mmap the cache file and return direct pointers to SQLite, eliminating pread syscalls for warm-cache reads (~5us to <1us per page).Tests
4 new tests in
tests/fetch_test.rs:Relationship to #80
Either PR fixes the SEGFAULT. #80 is the minimal fix (iVersion 3 -> 2). This PR is the proper fix (implement the methods that iVersion 3 declares). They're alternatives; merging either one works. If you prefer the minimal approach, merge #80 and this becomes a follow-up feature.