Repository navigation
sqlite: Honor page_size in document log streaming - #606
Open
Hades32sBot wants to merge 1 commit into
Open
Hades32sBot wants to merge 1 commit into
Hades32sBot wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The SQLite persistence layer ignored the page_size argument and eagerly materialized the entire queried timestamp range into a Vec before returning a stream. Since callers pass TimestampRange::all() with page_size=1 (e.g. PersistenceReader::max_ts() at startup), the backend loaded every row of the documents table - all MVCC revisions and tombstones - into memory twice on every boot. The same eager collection applied to retention passes over large backlogs, which is the OOM pattern reported in convex#358. Fetch rows in pages instead: keyset-paginate over the (ts, table_id, id) primary key with a LIMIT per query, holding only one page in memory and releasing the connection lock between pages. page_size remains a pure fetch-chunk-size hint, so results are unchanged.
Hades32sBot
force-pushed
the
sqlite-doclog-paging
branch
from
October 14, 2026 09:10
f4924af to
2ffe557
Compare
Author
|
@goffrie could you have a look? |
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.
Problem
SqlitePersistence::stream_document_logignorespage_sizeand eagerly materializes the entire queried timestamp range into aVecbefore returning a stream.The worst instance:
PersistenceReader::max_ts()callsload_documents(TimestampRange::all(), Order::Desc, page_size = 1)duringDatabase::load— so on every boot, the backend parses every row of thedocumentstable (all live docs plus all MVCC revisions and tombstones) intoConvexValues, twice (once vianew_idle_repeatable_ts(), once viaDatabaseSnapshot::max_ts). On a self-hosted instance with a few hundred MB of document history this allocates ~1.7 GB of heap and dominates startup time. The same eager collection applies to the retention worker'sexpired_documentsscans, which is the OOM pattern reported in #358.Change
Fetch rows in pages of
page_sizeinstead:load_docsgains aLIMITclause and a keyset-pagination clause over the(ts, table_id, id)primary key ((ts, table_id, id) > / < (?, ?, ?)matching theORDER BYdirection). When resuming, the redundant range bound on the ordering side is omitted so SQLite seeks into the PK index instead of re-scanning preceding rows on every page; for tablet-filtered scans+table_id = ?keeps the planner on the PK index.stream_document_log_pages(new#[try_stream]method) loops: run oneLIMIT page_sizequery per page (prepare_cached), hold only that page in memory, and release theMutex<Connection>between pages — which also lets writers interleave during long scans.retention_validator.validate_document_snapshot(min_ts)runs once upfront and after each page, since retention may advance while the lock is released between pages.load_documents/load_documents_from_tablepasspage_sizethrough;page_size.max(1)guards against a 0 page size.page_sizeremains a pure fetch-chunk-size hint per the trait docs, so stream contents and ordering are unchanged. Note the stream is no longer a single-statement snapshot across pages — matching the Postgres/MySQL drivers' contract.Impact
max_ts()at boot now reads 1 row instead of the whole table: startup memory is roughlyO(page_size)instead ofO(documents JSON size), which measured ~1.7 GB on a real self-hosted DB.Testing
cargo check/cargo clippy/rustfmt --checkclean for-p sqlite. No sqlite-persistence unit tests exist in the repo; the pagination logic was verified against a ~1.2M-row production-shaped DB copy and exercised exhaustively (all orders/filters/page sizes/ranges incl.page_size=0) against a brute-force reference.cc @Hades32