treedb: honor checksum-skip for value-log leafref reads#841
treedb: honor checksum-skip for value-log leafref reads#841snissn wants to merge 3 commits intopr/entry-slice-telemetry-countersfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aligns TreeDB’s value-log “leaf-ref” read paths with the existing “disable read checksum” policy (used by fast / wal_on_fast profiles and -treedb-disable-read-checksum). Previously, disabling read checksums skipped value-log record CRC validation but still verified leaf page checksums for leaf pages stored in the value log.
Changes:
- Added an optional
ReadChecksumEnabled()capability on slab/value-log readers and plumbed it through DB/value-log reader types. - Updated Tree hot read paths (
GetAppendand iterator leaf-ref reads) to conditionally skip leaf page checksum verification when read checksums are disabled. - Added/extended tests to cover enabled vs disabled behavior, including unified-bench profile assertions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
TreeDB/tree/tree.go |
Adds checksum-capability probing and gates leaf-ref page checksum verification on the configured policy. |
TreeDB/tree/iterator.go |
Applies the same conditional checksum verification for iterator leaf-ref page loads. |
TreeDB/db/value_reader.go |
Forwards ReadChecksumEnabled() from underlying value-log reader to the tree layer. |
TreeDB/internal/valuelog/manager.go |
Exposes ReadChecksumEnabled() on Manager and snapshot Set to reflect the disable-read-checksum setting. |
TreeDB/tree/tree_test.go |
Adds GetAppend coverage for leaf-ref checksum enabled/disabled behavior using a corrupted leaf page. |
TreeDB/tree/iterator_test.go |
Adds iterator coverage for leaf-ref checksum enabled/disabled behavior using a corrupted leaf page. |
cmd/unified_bench/profiles_treedb_index_test.go |
Asserts that fast and wal_on_fast profiles disable read checksums. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48766e1874
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f68ec13fa8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR makes TreeDB’s leaf-ref (outer-leaf-in-vlog) read paths honor the same “skip read checksums” policy already used for value-log record CRC verification (e.g., fast / wal_on_fast profiles and -treedb-disable-read-checksum), and adds regression tests to ensure iterator and GetAppend behavior matches the configured policy.
Changes:
- Add an optional
ReadChecksumEnabled()capability on slab/value-log readers and plumb it through value-log reader layers into tree/iterator leaf-ref page verification. - Conditionally skip
Node.VerifyChecksum()for leaf pages loaded via value-log leaf refs when read checksums are disabled. - Add tests for enabled vs disabled checksum behavior across
GetAppendand iterator leaf-ref reads, plus unified-bench profile assertions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
TreeDB/tree/tree.go |
Adds checksum-capability plumbing and gates leaf-ref page checksum verification in hot paths; adjusts Get return behavior. |
TreeDB/tree/iterator.go |
Gates iterator leaf-ref page checksum verification based on the tree’s checksum policy. |
TreeDB/tree/tree_test.go |
Adds regression test ensuring GetAppend leaf-ref checksum policy is honored. |
TreeDB/tree/iterator_test.go |
Adds regression test ensuring iterator leaf-ref checksum policy is honored. |
TreeDB/db/value_reader.go |
Exposes ReadChecksumEnabled() via the DB’s slab reader wrapper so tree code can observe policy. |
TreeDB/internal/valuelog/manager.go |
Implements ReadChecksumEnabled() on Manager and snapshot Set; minor robustness tweaks in stats helpers. |
cmd/unified_bench/profiles_treedb_index_test.go |
Asserts fast/wal_on_fast profiles disable read checksums. |
TreeDB/caching/expvar_stats.go |
Refactors process-wide expvar key selection into a helper predicate. |
TreeDB/caching/db.go |
Adds per-DB append-only allocation counters and changes which counters are surfaced under certain stats keys. |
Comments suppressed due to low confidence (1)
TreeDB/caching/db.go:17058
- The
treedb.process.append_only.mutable_new_alloc_with_queue_total/*_queue_bytes_sumstats are still sourced from global process-wide counters, while the correspondingtreedb.cache.append_only.*stats just above are sourced from DB-local atomics. Since othertreedb.process.append_only.*keys in this block mirror the DB-local values, this mixed sourcing is likely to confuse consumers. Consider making the process+cache variants consistently DB-local or consistently process-global (and adjust naming/tests accordingly).
stats["treedb.process.append_only.mutable_from_lease_total"] = fmt.Sprintf("%d", appendOnlyMemLeaseHitTotal)
stats["treedb.process.append_only.mutable_from_pool_total"] = fmt.Sprintf("%d", appendOnlyMemPoolHitTotal)
stats["treedb.process.append_only.mutable_new_alloc_total"] = fmt.Sprintf("%d", appendOnlyMemNewAllocTotal)
stats["treedb.process.append_only.mutable_new_alloc_with_queue_total"] = fmt.Sprintf("%d", appendOnlyMemNewAllocWithQueueTotal.Load())
stats["treedb.process.append_only.mutable_new_alloc_queue_bytes_sum"] = fmt.Sprintf("%d", appendOnlyMemNewAllocQueueBytesSum.Load())
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Summary
This PR makes TreeDB leaf-ref read paths honor the existing checksum-skip policy used by
fast/wal_on_fastprofiles and-treedb-disable-read-checksum.Before this change, value-log record CRC verification was skipped, but leaf pages loaded through value-log leaf refs were still always checksum-verified in hot read paths.
What changed
GetAppendand iterator leaf-ref reads.fastandwal_on_fastread-checksum behavior.Files
TreeDB/tree/tree.goTreeDB/tree/iterator.goTreeDB/tree/tree_test.goTreeDB/tree/iterator_test.goTreeDB/db/value_reader.goTreeDB/internal/valuelog/manager.gocmd/unified_bench/profiles_treedb_index_test.goValidation
Local tests
go test ./TreeDB/tree -run 'LeafRefChecksumPolicy|UsesAppendReaderForLeafRefPages' -count=1go test ./cmd/unified_bench -run 'ApplyProfile_FastAndWALOnFastEnableIndexOptimizations' -count=1go test ./TreeDB/db -run '^$' -count=1go test ./TreeDB/internal/valuelog -run '^$' -count=1go test ./TreeDB/caching -run '^$' -count=1go vet ./...unified-bench A/B (same command)
Command:
GOWORK=off make unified-bench && ./bin/unified-bench -dbs treedb -profile fast -keys 500000 -progress=false -treedb-index-outer-leaves-in-vlog=true -valsize=100 -checkpoint-between-tests -treedb-force-value-pointers=false -profile-dir=.../home/mikers/tmp/leafcrc-fast-patch-1773798543/home/mikers/tmp/leafcrc-fast-base-1773798579Read-path deltas (patch vs base):
random_read:+8.45%(409,912 -> 444,538)random_read_parallel:+2.28%(3,565,184 -> 3,646,320)random_read_batch:+6.39%(2,391,728 -> 2,544,510)full_scan:+4.29%prefix_scan:+3.91%random_read_parallel_acquire_snapshot:-0.55%CPU profile signal:
Node.VerifyChecksum/page.CalculateChecksum/hash/crc32.*~4.26% cum).run_celestia (serial)
/home/mikers/.celestia-app-mainnet-treedb-20260317155044/sync/sync-time.logduration_seconds=293max_rss_kb=10456660end_app_bytes=5864844315/home/mikers/.celestia-app-mainnet-treedb-20260317155552/sync/sync-time.logduration_seconds=380max_rss_kb=12108616end_app_bytes=6197787609/home/mikers/.celestia-app-mainnet-goleveldb-20260317160312/sync/sync-time.logduration_seconds=755max_rss_kb=11686680end_app_bytes=2951364090