db: Compute post-apply checksum incrementally#449
Conversation
TruncateWAL already clears the checksums
We do not make use of older checksum values
saleemrashid
force-pushed
the
saleem/incremental-chksum
branch
4 times, most recently
from
766edc5 to
df75264
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates checksum handling to avoid full database checksum recomputation by computing post-apply checksums incrementally during WAL & rollback-journal commits.
Changes:
- Replace rollback-journal dirty page tracking with a map of previous per-page checksums to support incremental checksum updates.
- Simplify WAL per-page checksum tracking to store only the latest checksum per page.
- Remove the “in-progress WAL checksums” path from
checksum()/pageChecksum()and compute post-apply checksum via XOR-deltas during commit.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
saleemrashid
force-pushed
the
saleem/incremental-chksum
branch
4 times, most recently
from
e077010 to
ac2bef2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Disable page cache so dirty pages are written immediately. | ||
| if _, err := db.Exec(`PRAGMA cache_size = 0`); err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
PRAGMA cache_size = 0 does not disable SQLite's page cache; it resets the cache size to the default. If the intent is to force more frequent page flush/eviction so the same pages are written multiple times within a transaction, consider using a very small cache (e.g., 1–2 pages) or update the comment to match the actual behavior.
There was a problem hiding this comment.
Does not seem to be the case? https://github.com/sqlite/sqlite/blob/a5495355c48e65411db52ac88d5b361d4d2c9f35/test/cache.test#L117
saleemrashid
changed the title
saleemrashid
force-pushed
the
saleem/incremental-chksum
branch
from
ac2bef2 to
fa7bad4
Compare
| // This store will automatically close when the test ends. | ||
| func newStore(tb testing.TB, leaser litefs.Leaser, client litefs.Client) *litefs.Store { | ||
| store := litefs.NewStore(tb.TempDir(), true) | ||
| store.StrictVerify = true |
There was a problem hiding this comment.
This doesn't actually do anything for these tests, so we could drop this commit.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Update WAL checksums. | ||
| for pgno, chksum := range newWALChksums { | ||
| db.wal.chksums[pgno] = append(db.wal.chksums[pgno], chksum) | ||
| db.wal.chksums[pgno] = chksum | ||
| } |
There was a problem hiding this comment.
CommitWAL() stores 0 values in db.wal.chksums (e.g. for truncated pages). Because checksum() ranges over the keys in db.wal.chksums to mark ignored blocks, leaving tombstone entries can (a) keep blocks permanently marked as ignored and (b) create invalid pgno keys (> current PageN()) after a truncating commit, which can lead to out-of-range indexing in checksum(). Prefer deleting entries when the WAL no longer contains a page (e.g. use delete(db.wal.chksums, pgno) when chksum == 0), or filter pgno keys against pageN in checksum().
| // Remove WAL checksums. These shouldn't exist but remove them just in case. | ||
| db.wal.chksums = make(map[uint32][]ltx.Checksum) | ||
| db.wal.chksums = make(map[uint32]ltx.Checksum) | ||
|
|
||
| // Compute new database checksum. | ||
| postApplyChecksum := prevPos.PostApplyChecksum |
There was a problem hiding this comment.
CommitJournal() clears db.wal.chksums but leaves db.wal.frameOffsets intact. These two structures both represent WAL overlays; clearing only one can leave the DB in an inconsistent state (e.g. onDiskChecksum(..., walFile=nil) will still try to read from WAL if frameOffsets contains keys). Consider clearing db.wal.frameOffsets (and any other WAL overlay state) alongside db.wal.chksums here to avoid stale WAL references when processing rollback-journal commits or mode switches.
This change computes the post-apply checksum incrementally in
CommitJournalandCommitWAL, keeping commit times proportional to the transaction size, rather than the database size or the WAL size (which quickly causes a performance cliff, because it invalidates the cached block checksums).With a 30GiB database, our commit times have been increasing, and we see some start to exceed 30 seconds when a long-running read transaction (particularly a LiteFS snapshot) causes the WAL to grow to above 70MiB.
Since
ApplyLTXNoLockhasn't been changed, replicas will still be verifying the post-apply checksum with the existingchecksummethod, which should at least allow us to detect if there's any bugs in the incremental checksumming that we haven't caught.The increased test coverage also caught a bug where switching between WAL and journal mode results in some pages not being written to the LTX.