Add read-scoped block buffers for scan reads

[xingbowang]

Summary

• Add read-scoped block buffers for scan reads. Introduce an experimental ReadScopedBlockBufferProvider API and ReadOptions::read_scoped_block_buffer_provider so block-based table iterator and MultiScan data-block reads can use caller-provided read-scoped storage. Provider-backed scan reads bypass block cache, attach provider cleanup to uncompressed read buffers without copying, and decompress compressed blocks directly into provider-backed output.

• Add an independent MultiScanArgs::bypass_block_cache / IODispatcher JobOptions bypass knob for scan reads that should skip data-block cache lookup and insertion while preserving fill_cache=false behavior.

• Extend AlignedBuffer and RandomAccessFileReader direct-I/O paths to support external aligned allocations, then use that support for read-scoped iterator, async I/O, and MultiRead scratch buffers. Centralize read-scoped I/O policy, keep coalesced async reads safe when blocks are released before completion, and validate provider lease contracts.

• Add focused coverage for read-scoped ownership, compressed and uncompressed blocks, direct I/O, cache bypass behavior, invalid provider leases, async release handling, and stress-test provider invariants. Add public API release notes for read-scoped block buffers.

Testing

• CI

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 432.1s.

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit c5f5998

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

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

Commands:

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

[meta-codesync]

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

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit c5f5998

---

Summary

This PR adds a well-structured ReadScopedBlockBufferProvider API for caller-managed block storage in scan reads, plus bypass_block_cache for MultiScan. The core design is sound, but there are several correctness issues in the IODispatcher integration paths and test coverage gaps.

High-severity findings (3):

• [io_dispatcher_imp.cc:375-380] ReadSet::SyncRead() hardcodes use_cache=true, ignoring bypass_block_cache and provider settings for fallback reads.
• [io_dispatcher_imp.cc:1057-1060] ExecuteSyncIO() still uses AlignedBuf aligned_buf with the old MultiRead(AlignedBuf*) API, inconsistent with the new AlignedBuffer& signature.
• [io_dispatcher_imp.cc:706-715] SubmitJob calls LookupAndPinBlocksInCache unconditionally even when bypass_block_cache=true, defeating the purpose of cache bypass.

Full review (click to expand)

Findings

🔴 HIGH

H1. ReadSet::SyncRead() ignores bypass_block_cache and provider -- io_dispatcher_imp.cc:375-380

• Issue: SyncRead() is the fallback when a block isn't prefetched or async IO fails. It hardcodes use_cache=true and use_block_cache_for_lookup=true in the call to RetrieveBlock. When bypass_block_cache=true or a ReadScopedBlockBufferProvider is configured, this sync fallback path will still insert/lookup blocks in the block cache, creating inconsistent behavior between prefetched and on-demand reads within the same scan.
• Root cause: SyncRead was written before bypass_block_cache existed and was not updated by this PR.
• Suggested fix: Compute use_cache using ShouldUseBlockCacheForIteratorDataBlocks(rep->table_options, job_->job_options.read_options, job_->job_options.bypass_block_cache) and pass that to RetrieveBlock.

H2. ExecuteSyncIO() uses stale AlignedBuf API -- io_dispatcher_imp.cc:1057-1060

• Issue: The diff changes MultiRead to take AlignedBuffer& instead of AlignedBuf*, but ExecuteSyncIO() still uses AlignedBuf aligned_buf and passes direct_io ? &aligned_buf : nullptr. Since the signature changed to AlignedBuffer&, this code will fail to compile or silently use the wrong overload. Additionally, when a ReadScopedBlockBufferProvider is configured, the sync IO path should use provider-backed storage for direct IO reads.
• Root cause: ExecuteSyncIO was not updated to match the new MultiRead signature. The diff is truncated so we cannot confirm this was addressed later.
• Suggested fix: Replace AlignedBuf aligned_buf with AlignedBuffer direct_io_buffer and wire up provider-backed allocation for the sync path.

H3. SubmitJob calls LookupAndPinBlocksInCache when bypass_block_cache=true -- io_dispatcher_imp.cc:706-715

• Issue: SubmitJob iterates all blocks and calls LookupAndPinBlocksInCache unconditionally. When bypass_block_cache=true, this performs unnecessary cache lookups for every block, adding overhead to a path specifically designed to avoid cache interaction.
• Root cause: SubmitJob was not updated to check bypass_block_cache before cache lookup.
• Suggested fix: Guard the cache lookup with ShouldUseBlockCacheForIteratorDataBlocks() or check job->job_options.bypass_block_cache before calling LookupAndPinBlocksInCache.

🟡 MEDIUM

M1. CreateAndPinBlockFromBuffer -- missing read_buffer_cleanup at call sites

• Issue: The diff adds const SharedCleanablePtr& read_buffer_cleanup to CreateAndPinBlockFromBuffer. The PollAndProcessAsyncIO and ExecuteSyncIO call sites need updating to pass the appropriate cleanup for provider-backed reads. Without this, provider-backed uncompressed blocks in the no-cache path will fail with "read-scoped block buffer provider requires read buffer cleanup".
• Suggested fix: Ensure both call sites pass the appropriate cleanup from the read buffer's provider lease.

M2. std::function overhead in AlignedBuffer::Allocator -- util/aligned_buffer.h

• Issue: AlignedBuffer::Allocator uses std::function with type erasure overhead. RocksDB typically avoids std::function on performance-sensitive paths.
• Suggested fix: Consider a function pointer + context pair, or document that this is acceptable for the scan/provider path.

M3. AsyncIOState missing provider wiring -- io_dispatcher_imp.cc:957-1015

• Issue: ExecuteAsyncIO still uses the old aligned_buf field and doesn't wire up provider-backed allocation for async reads with direct IO.
• Suggested fix: Wire up MakeReadScopedAlignedBuffer for the async direct IO path.

M4. Hot-path shared_ptr check in GetReadScopedBlockBufferProvider

• Issue: GetReadScopedBlockBufferProvider checks a shared_ptr on every block read via ShouldUseBlockCacheForIteratorDataBlocks. Consider caching at iterator construction.

🟢 LOW / NIT

L1. ReadAsync parameter ordering -- random_access_file_reader.h

• Issue: direct_io_buffer added after dbg creates non-intuitive ordering of two defaulted parameters.

L2. bypass_block_cache vs fill_cache naming confusion -- options.h

• Issue: The dual cache control mechanisms could confuse users. Consider cross-referencing in docs.

Cross-Component Analysis

Context	Code executes?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES (scans)	YES - provider is read-path only	Safe
ReadOnly DB	YES	YES	Safe
CompactionService	NO	N/A	Safe
User-defined timestamps	YES	YES - orthogonal	Safe
BlobDB	NO - separate path	N/A	Safe
Concurrent iterators	YES	Provider must be thread-safe	Per API contract
Direct IO + non-aligned reads	YES	Alignment contract covers this	Verify with tests
Immortal tables (mmap)	YES	Provider bypasses cache, mmap may still be used	Verify block_contents_pinned

Positive Observations

• Well-designed ReadScopedBlockBufferProvider API with clear ownership semantics via SharedCleanablePtr
• AllocateReadScopedBlockBuffer validation provides defense-in-depth against misbehaving providers
• StressReadScopedBlockBufferProvider in db_stress provides comprehensive invariant checking
• AlignedBuffer::AllocateNewBufferWithStatus is a clean extension for external allocation
• LookupAndPinBlocksInCache graceful null handling is the right approach
• Good test coverage for direct IO external buffer paths

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.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

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit cbf3237

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

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

Commands:

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

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit cbf3237

---

Summary

Large, well-structured PR that introduces read-scoped block buffer providers and cache bypass for scan reads. The core concept is sound and the implementation covers many paths (direct I/O, async I/O, compressed/uncompressed blocks, stress testing). However, several correctness gaps exist where the new bypass and provider logic is not consistently applied across all read paths, and the IODispatcher sync fallback ignores the new settings entirely.

High-severity findings (3):

• [io_dispatcher_imp.cc: SyncRead] ReadSet::SyncRead() hardcodes use_cache=true, ignoring bypass_block_cache and provider settings when async I/O falls back to sync.
• [io_dispatcher_imp.cc: SubmitJob] SubmitJob() unconditionally calls LookupAndPinBlocksInCache for all blocks, performing cache lookups even when bypass_block_cache=true.
• [io_dispatcher_imp.cc: ExecuteSyncIO] ExecuteSyncIO() still uses the old AlignedBuf type and passes pointer/nullptr to MultiRead, not the new AlignedBuffer& reference. This will fail to compile against the changed MultiRead signature.

The full review with 5 medium and 5 low findings, cross-component analysis, and positive observations is in review-findings.md.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.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

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit dc9c2e6

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

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

Commands:

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

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit dc9c2e6

---

Summary

Large, well-structured PR that introduces read-scoped block buffer providers for scan reads, allowing caller-provided storage to bypass the block cache. The refactoring of AlignedBuf to AlignedBuffer for direct I/O paths is clean and the stress test provider is thorough. However, there are correctness issues in the IODispatcher integration that need attention.

High-severity findings (2):

• [util/io_dispatcher_imp.cc:SyncRead] ReadSet::SyncRead() hardcodes use_cache=true, defeating bypass_data_block_cache and provider-based cache bypass for blocks that fall back to synchronous reads.
• [util/io_dispatcher_imp.cc:SubmitJob] SubmitJob() unconditionally calls LookupAndPinBlocksInCache() even when bypass_data_block_cache=true, performing unnecessary cache lookups that the feature is designed to avoid.

Full review (click to expand)

Findings

🔴 HIGH

H1. ReadSet::SyncRead hardcodes use_cache=true, bypassing cache bypass logic -- util/io_dispatcher_imp.cc:356

• Issue: The existing ReadSet::SyncRead() method calls RetrieveBlock<Block_kData> with use_cache=true and use_block_cache_for_lookup=true. When bypass_data_block_cache=true or a read_scoped_block_buffer_provider is configured, blocks that cannot be served from async I/O or the initial SubmitJob dispatch fall back to SyncRead() via ReadIndex(). These blocks will be read through the cache, violating the caller's intent. The NewDataBlockIterator() path correctly uses ShouldUseDataBlockCacheForIterator(), but SyncRead() does not.
• Root cause: The SyncRead fallback path was not updated to respect the new cache bypass and provider configurations.
• Suggested fix: Compute use_cache using ShouldUseDataBlockCacheForIterator(rep->table_options, job_->job_options.read_options, job_->job_options.bypass_data_block_cache) and pass it to RetrieveBlock. When not using cache, also pass the ReadScopedBlockBufferProviderRef to enable provider-backed block storage.

H2. SubmitJob unconditionally performs cache lookup even with bypass -- util/io_dispatcher_imp.cc:713

• Issue: In SubmitJob(), the loop at line 709 calls LookupAndPinBlocksInCache() for every block handle regardless of bypass_data_block_cache or provider configuration. While the PR correctly changes LookupAndPinBlocksInCache() to handle a null block cache (by returning OK early), when a block cache IS configured but bypass_data_block_cache=true, the function still performs a full cache lookup (including cache key computation, statistics updates, and potential hash table lookup). This is unnecessary overhead for the bypass case and could pollute cache statistics.
• Root cause: The SubmitJob method pre-dates the bypass feature and was not updated to skip cache lookups.
• Suggested fix: Guard the cache lookup with the same ShouldUseDataBlockCacheForIterator() check. When bypassing, add all blocks directly to block_indices_to_read without attempting cache lookup.

🟡 MEDIUM

M1. ReadSet destructor memory release logic change may double-release -- util/io_dispatcher_imp.cc:~ReadSet

• Issue: The old destructor guarded memory release with if (auto dispatcher_data = dispatcher_data_.lock()) and checked block_sizes_[i] > 0 && pinned_blocks_[i].GetValue(). The new code checks pinned_blocks_[i].GetValue() || async_io_map_.find(i) != async_io_map_.end() and calls ReleasePrefetchMemory(i). For blocks that were sync-read during ReadIndex() fallback (Case 3 in ReadIndex), block_sizes_[i] may be set (from SubmitJob) but TryAcquireMemory() was never called. The new destructor would call ReleasePrefetchMemory() for these blocks, potentially releasing memory that was never acquired, corrupting the dispatcher's memory accounting.
• Suggested fix: ReleasePrefetchMemory() should check whether memory was actually acquired for this block before calling dispatcher_data->ReleaseMemory(). Consider adding a per-block flag indicating whether memory was acquired.

M2. GetBlockContents has_trailer not set for provider-backed uncompressed blocks -- table/block_fetcher.cc:GetBlockContents

• Issue: In the new provider path within GetBlockContents(), when read_scoped_buf_lease_.cleanup.get() != nullptr && compression_type() == kNoCompression, the code sets contents_->data and contents_->cleanup but does NOT set contents_->has_trailer (the #ifndef NDEBUG block at the end of GetBlockContents only runs for the else branch that goes through heap_buf_). The provider path returns early via the else if branch, skipping the final has_trailer assignment. This could cause debug-mode assertion failures in code that expects has_trailer to be set.
• Suggested fix: Add #ifndef NDEBUG contents_->has_trailer = footer_.GetBlockTrailerSize() > 0; #endif to the provider branch, matching the pattern used in the non-provider code paths.

M3. AlignedBuffer::AllocateNewBufferWithStatus leaves stale state on error -- util/aligned_buffer.h:AllocateNewBufferWithStatus

• Issue: If the external allocator returns an error (null data, short buffer, misaligned, etc.), the function returns the error Status but does NOT reset bufstart_, capacity_, or buf_ from their previous values. If the AlignedBuffer had a previous allocation, it retains that stale state. Subsequent calls to BufferStart() or Capacity() would return the old buffer's values, which could lead to use-after-free if the old buffer was already released.
• Suggested fix: On error, leave the AlignedBuffer state unchanged (which the current code already does -- the issue is only if the allocator partially modifies buf_ via the owner field before returning error). Alternatively, clear internal state on error.

M4. std::shared_ptr in ReadOptions adds atomic overhead on every copy -- include/rocksdb/options.h:ReadOptions

• Issue: read_scoped_block_buffer_provider is a std::shared_ptr in ReadOptions. ReadOptions is copied frequently in hot paths (e.g., per-iterator creation, per-MultiGet call). Each copy increments/decrements an atomic reference count. This is the first shared_ptr field in ReadOptions, setting a precedent that could accumulate overhead.
• Suggested fix: Consider using a raw pointer or UnownedPtr with the caller responsible for lifetime, similar to table_index_factory. Alternatively, document that this is intentional for the experimental API.

🟢 LOW / NIT

L1. BlockContents default constructor leaves backing_size uninitialized implicitly -- table/format.h:BlockContents

• Issue: size_t backing_size = 0 has an in-class initializer, but the copy of BlockContents(const Slice& _data) constructor does not mention it. The in-class initializer handles this correctly in C++11+, but it may be clearer to be explicit in the move assignment operator comment.

L2. CreateAndPinBlockFromBuffer test sync point captures by value unnecessarily -- util/io_dispatcher_imp.cc:CreateAndPinBlockFromBuffer

• Issue: The #ifndef NDEBUG block creates test_read_buffer_cleanup as a copy of read_buffer_cleanup, then uses effective_read_buffer_cleanup to switch between test and production. The copy involves atomic ref count operations. This is debug-only so performance is not critical, but the pattern is unnecessarily complex.

L3. NOLINTNEXTLINE comment on stress gflag -- db_stress_tool/db_stress_gflags.cc

• Issue: Only one of the two new DEFINE_bool macros has a NOLINTNEXTLINE suppression. The read_scoped_block_buffer_provider flag definition is missing it, which may trigger a clang-tidy warning.

L4. Missing db_bench support for read_scoped_block_buffer_provider -- tools/db_bench_tool.cc

• Issue: The PR adds bypass_data_block_cache to db_bench but does NOT add support for configuring read_scoped_block_buffer_provider in db_bench. For benchmarking the provider feature, users would need to modify db_bench. Consider adding a built-in simple provider for benchmarking.

Cross-Component Analysis

Context	Affected?	Analysis
WritePreparedTxnDB	No	Provider only affects read paths, not visibility or transactions
ReadOnly DB / SecondaryInstance	Safe	Provider-backed reads work identically to normal reads
CompactionService / Remote	No	Provider is on ReadOptions which is per-read, not serialized
User-defined timestamps	Safe	Provider is orthogonal to key ordering
BlobDB	Safe	Blob reads use separate BlobFileReader::ReadFromFile, which was correctly updated
FIFO / Universal compaction	No	Provider only affects scan reads, not compaction
Prefix seek	Safe	Provider is block-level, independent of seek mode
Concurrent writers	Safe	Provider allocations use per-provider synchronization
Snapshots	Safe	Provider is memory management, not visibility

Positive Observations

• The StressReadScopedBlockBufferProvider is well-designed with comprehensive invariant checking (alignment, size, lifecycle tracking, abort on violation).
• The AlignedBuffer external allocator design cleanly separates allocation policy from the buffer management, enabling provider-backed direct I/O without duplicating the read loop.
• The AllocateReadScopedBlockBuffer() validation function properly checks all lease contract requirements (non-null data, sufficient size, alignment, non-null cleanup).
• The refactoring from AlignedBuf (type alias for unique_ptr<void>) to AlignedBuffer (full class) for direct I/O paths is a good cleanup that makes the ownership model more explicit.
• The ShouldUseDataBlockCacheForIterator() centralization is a good pattern for consistent cache bypass decisions.
• Compressed block decompression into provider-backed memory avoids an extra copy that would otherwise be needed.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.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