Cross-key coalescing for lazy wide-column blob reads

[pdillinger]

Summary:
Follow-up (Phase 3) to the experimental lazy wide-column read API (DB::GetEntityLazy / DB::MultiGetEntityLazy). A MultiGetEntityLazy batch now resolves blob references across keys with coalesced I/O instead of key-by-key.

• MultiGetEntityLazy acquires one SuperVersion and one consistent (implicit) sequence number for the whole batch via MultiCFSnapshot (as multi-CF iterators do) and transfers a single shared per-column-family pin into the LazyWideColumnsBatch, replacing the former per-key SuperVersion + explicit-snapshot loop. Single column family for now; a cross-CF ColumnFamily** overload remains a follow-up (the batch already models a per-CF pin map).
• LazyWideColumnsBatch::MultiResolve groups the batch's reads by (Version, blob file) for separate-file references and by SST for embedded references, and issues one coalesced read per group -- for whole-column and byte-range reads alike -- caching whole values in each entity's resolver and pinning partial reads directly.
• Adds the coalesced blob-read primitives this needs (none existed before): range-aware BlobFileReader::MultiGetBlobRange / BlobSource::MultiGetBlobRange / Version::MultiGetBlobLazy for separate-file references, and an embedded (SimpleGen2) batch read path (blob_gen2_format batch readers + BlobSource::MultiGetSimpleGen2Blob[Range] + BlockBasedTable::MultiGetSameFileBlob). Whole-value separate-file coalescing reuses the existing MultiGetBlob. The batch readers sort their requests by file offset before issuing the underlying MultiRead (which requires ascending offsets), and gen2 checksum verification is factored into one helper shared by the scalar and batch readers.
• The hot non-lazy Get / MultiGet / iterator / compaction paths are untouched: the new multis are lazy-only, and the GetImpl change is gated on a new opt-in GetImplOptions field (an injected shared SuperVersion + sequence number) used only by the batched lazy read.
• Blob read failures on these paths (short reads, invalid offsets, out-of-range requests, compression mismatches) now carry file/offset/size and expected-vs-actual context, mirroring the block-checksum diagnostics.
• db_bench gains a multireadrandomentitylazy benchmark for measuring the coalesced batch path.

No public API change, so this is a performance follow-up to the still-recent lazy API rather than a new feature.

Test Plan:
New unit tests in db/wide/db_lazy_entity_test.cc assert cross-key coalescing via a blob-file / SST MultiRead counter added to the test FileSystem wrapper:

• N whole-column separate-file reads across keys in one blob file collapse to a single coalesced MultiRead (and the fetched values are cached, so a repeat read does no further blob I/O);
• N byte-range separate-file reads collapse to one MultiRead, save the un-read bytes (rocksdb.blobdb.lazy.partial.bytes.saved), and never fill the blob cache;
• embedded (same-file) reads across keys in one SST collapse to one MultiRead over that SST;
• an out-of-order embedded batch (reads issued in descending record offset) still resolves correctly in one coalesced read, covering the batch readers' internal offset sort;
• a mixed batch (inline + whole + byte-range across keys) returns the correct bytes for each read;
• a batched MultiGetEntityLazy + one MultiResolve matches resolving each key via a separate GetEntityLazy. The existing db_stress lazy-vs-eager differential (MaybeTestMultiGetEntityLazy) exercises the new batched read and cross-key MultiResolve unchanged (same public API).

[meta-codesync]

@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D118660682.

[github-actions]

⚠️ clang-tidy: 9 warning(s) on changed lines

Completed in 533.3s.

Summary by check

Check	Count
bugprone-argument-comment	1
cppcoreguidelines-pro-type-member-init	1
modernize-use-emplace	5
performance-inefficient-vector-operation	2
Total	9

Details

db/blob/blob_file_reader.cc (1 warning(s))

db/blob/blob_file_reader.cc:832:33: warning: argument name 'allocator' in comment does not match parameter name 'alloc' [bugprone-argument-comment]

db/db_impl/db_impl.cc (1 warning(s))

db/db_impl/db_impl.cc:2963:3: warning: uninitialized record type: 'cf_sv_pairs' [cppcoreguidelines-pro-type-member-init]

db/wide/db_lazy_entity_test.cc (7 warning(s))

db/wide/db_lazy_entity_test.cc:1059:12: warning: use emplace_back instead of push_back [modernize-use-emplace]
db/wide/db_lazy_entity_test.cc:1141:12: warning: use emplace_back instead of push_back [modernize-use-emplace]
db/wide/db_lazy_entity_test.cc:1212:12: warning: use emplace_back instead of push_back [modernize-use-emplace]
db/wide/db_lazy_entity_test.cc:1215:5: warning: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation]
db/wide/db_lazy_entity_test.cc:1273:12: warning: use emplace_back instead of push_back [modernize-use-emplace]
db/wide/db_lazy_entity_test.cc:1276:5: warning: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation]
db/wide/db_lazy_entity_test.cc:1380:12: warning: use emplace_back instead of push_back [modernize-use-emplace]

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 3479e8d

---

Summary

Well-designed PR that replaces the per-key SuperVersion + explicit-snapshot loop in MultiGetEntityLazy with a genuinely batched path using MultiCFSnapshot, and adds cross-key I/O coalescing for lazy blob resolution. The architecture is sound: shared SV pin lifetime management is correct, new batch read primitives follow existing patterns, and the hot non-lazy paths are untouched.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. ReadOnly/Secondary DB GetImpl not updated for lazy_columns_shared_sv — db_impl_readonly_sync_and_async.h, db_impl_secondary_sync_and_async.h

• Issue: The diff modifies DBImpl::GetImpl in db_impl_sync_and_async.h to handle lazy_columns_shared_sv (skip SV acquisition, use the shared SV, use lazy_columns_snapshot_seq as snapshot). However, DBImplReadOnly::GetImpl and DBImplSecondary::GetImpl have their own GetImpl copies that are NOT modified. If GetEntityLazyForBatch were ever called on a readonly/secondary DB, the shared SV would be ignored and a new one acquired, creating an inconsistency.
• Root cause: Each DB subclass has its own GetImpl implementation.
• Mitigating factor: GetEntityLazyForBatch and MultiGetEntityLazy are only defined on DBImpl, and neither DBImplReadOnly nor DBImplSecondary override them. Moreover, GetEntityLazy requires max_open_files == -1 and is currently only available on DBImpl. So this is not reachable today.
• Suggested fix: No immediate fix needed, but add a comment in GetEntityLazyForBatch noting the assumption that it is only called through DBImpl::GetImpl. If readonly/secondary lazy support is added later, the lazy_columns_shared_sv handling must be replicated.

M2. Version::MultiGetBlobLazy uses std::unordered_map for file grouping — version_set.cc

• Issue: whole_idx and range_idx are std::unordered_map<uint64_t, size_t> used to group requests by blob file number. For small batch sizes (MAX_BATCH_SIZE=32), the hash map overhead (heap allocation, hashing) may exceed the benefit vs. a linear scan or autovector-based approach.
• Root cause: Hash map chosen for O(1) lookup but with high constant overhead for small N.
• Suggested fix: Consider using a sorted autovector or a simple linear scan for grouping, since N <= 32. This is not on the hot path (lazy-only), so the impact is minor.

M3. BlobRangeReadRequest constructor takes _user_key by const ref but stores a pointer — blob_read_request.h

• Issue: The constructor BlobRangeReadRequest(const Slice& _user_key, ...) stores user_key(&_user_key). If the caller passes a temporary Slice, the stored pointer will dangle. This follows the existing BlobReadRequest pattern and callers pass long-lived Slices, but it's a latent footgun.
• Root cause: Raw pointer stored from a reference parameter.
• Suggested fix: Document the lifetime requirement in a comment. Consistent with existing BlobReadRequest, so no code change strictly needed.

M4. SameFileBlobReader::MultiGetSameFileBlob default loop may not match test expectations — same_file_blob_reader.h

• Issue: The default implementation loops calling GetSameFileBlob per-request (correct but not coalesced). The test BatchCoalescesEmbeddedReads asserts sst_multiread_count_ == 1, which requires a coalesced MultiRead. This would only pass if BlockBasedTable overrides MultiGetSameFileBlob with a coalescing implementation. The diff is truncated so this override may exist in the unshown portion.
• Root cause: Default virtual fallback doesn't coalesce.
• Suggested fix: Verify that BlockBasedTable::MultiGetSameFileBlob is implemented in the full PR. If it's only the default loop, the embedded coalescing tests will fail.

M5. Double sorting concern for separate-file range reads — blob_source.cc + blob_file_reader.cc

• Issue: BlobSource::MultiGetBlobRange sorts blob_reqs_in_file by effective read offset. BlobFileReader::MultiGetBlobRange has a debug assert checking sort order but does NOT re-sort. This is correct and efficient -- sorted once at the right layer. For the gen2 embedded path, MultiReadGen2 independently sorts because it serves both whole-record and range callers with potentially unsorted input. No actual double-sort bug exists.
• Suggested fix: None needed; noting for reviewer awareness that the sort responsibilities are cleanly separated.

🟢 LOW / NIT

L1. VerifySimpleGen2BlobChecksum formatting nit — blob_gen2_format.cc

• Issue: Error message says "stored" + std::string(modifier ? "(context removed)" : "") with no space before the parenthetical. Reads as "stored(context removed)" instead of "stored (context removed)".
• Suggested fix: "stored " + std::string(modifier ? "(context removed)" : "") or "stored" + std::string(modifier ? " (context removed)" : "").

L2. (void)sv_from_thread_local cast — db_impl.cc

• Issue: Uses (void)sv_from_thread_local; with a comment explaining why. This is correct but unconventional; some codebase areas use [[maybe_unused]] or simply omit the variable name.
• Suggested fix: Minor style nit; acceptable as-is.

L3. New error messages allocate strings on cold paths — blob_file_reader.cc, blob_gen2_format.cc

• Issue: The improved error messages use std::to_string() and string concatenation. This is fine since error paths are cold. The added diagnostics (file/offset/size/expected-vs-actual) significantly improve debuggability, mirroring the block-checksum diagnostic pattern.
• Suggested fix: None needed; positive change.

L4. LazyWideColumnsBatch::Rep destructor thread operation handling — lazy_wide_columns.cc

• Issue: The new Rep::~Rep saves/restores the thread operation (OP_UNKNOWN during cleanup), mirroring LazyWideColumns::Rep::~Rep and DBIter::~DBIter. Correct and defensive.
• Suggested fix: None needed; positive pattern.

Cross-Component Analysis

SuperVersion lifetime chain (verified correct):

1. MultiCFSnapshot acquires SV with extra_sv_ref=true → independent ref via GetReferencedSuperVersion
2. GetEntityLazyForBatch passes SV to GetImpl via lazy_columns_shared_sv → GetImpl does NOT acquire/release its own SV
3. After all keys: TransferSuperVersionPin(sv, BatchCfPin(result, cfd->GetID())) → Refs SV, registers cleanup on batch's cf_pin
4. CleanupSuperVersion(sv) → releases the call-scoped reference
5. Batch destruction: entities cleared first (resolvers reference the Version), then cf_pins cleared (SV cleanup runs)

The SV has exactly two references during the loop (MultiCFSnapshot's + any TransferSuperVersionPin'd), and exactly one after (in the batch's cf_pin).

Execution context analysis:

Context	Safe?	Reason
WritePreparedTxnDB	Yes	Public DB API, not used through txn layer
ReadOnly DB	N/A	MultiGetEntityLazy not available
Secondary Instance	N/A	Same
User-defined timestamps	Yes	Timestamp handling is after SV acquisition
Concurrent writers	Yes	MultiCFSnapshot handles flush races

Assumption stress-test:

• "MultiCFSnapshot provides equivalent guarantee to explicit snapshot": Verified. Both capture GetLastPublishedSequence(). MultiCFSnapshot retries on flush race. Semantically equivalent for reads.
• "Shared SV is safe across multiple GetImpl calls": Verified. GetImpl is purely read-only w.r.t. SuperVersion.
• "Entities destroyed before cf_pins in batch Rep": Verified by explicit entities.clear() before cf_pins.clear() in destructor + field declaration order.

Positive Observations

• Clean architecture: The batch coalescing classifies reads first, groups by storage location, then dispatches -- a well-structured pipeline that will compose cleanly with future async execution.
• Shared checksum helper (VerifySimpleGen2BlobChecksum): Eliminates divergence risk between scalar and batch readers. Good refactoring.
• Error diagnostics: Adding file/offset/size/expected-vs-actual context to every error path is a significant debuggability win.
• Test design: The BlobReadIOActivityFS wrapper with per-file-type read/multiread counters effectively verifies I/O coalescing at the system level.
• MultiCFSnapshot reuse: Architecturally consistent with multi-CF iterators; avoids DB mutex overhead of explicit snapshots.
• Defensive destruction: Thread operation save/restore in batch Rep destructor prevents I/O misattribution.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase