Fix CompactedDBImpl blob values unreadable via read-only Get/MultiGet

[dfa1]

Summary

Fixes #12503.

CompactedDBImpl::Get and CompactedDBImpl::MultiGet (the fast path DB::OpenForReadOnly takes when max_open_files == -1 and the DB has at most one file per level) pass a null is_blob_index pointer into GetContext. Any key resolving to a blob index therefore hits GetContext::kUnexpectedBlobIndex immediately, even though a VersionBlobFetcher is already constructed and passed in -- the fetcher is unreachable because GetContext::SaveValue bails out for kTypeBlobIndex before ever reaching the branch that would use it, unless is_blob_index_ is non-null. The key is reported as Status::NotFound(), even though it and its blob file are both present.

Path db_dir = ...;
Options options;
options.enable_blob_files = true;
options.min_blob_size = 0;
// options.max_open_files defaults to -1

{
  DB* db;
  DB::Open(options, db_dir, &db);
  db->Put(WriteOptions(), "key", std::string(1000, 'a'));
  db->Flush(FlushOptions());  // single L0 file -> qualifies for CompactedDBImpl
  delete db;
}

DB* db;
DB::OpenForReadOnly(options, db_dir, &db);  // silently opens as CompactedDBImpl
std::string value;
db->Get(ReadOptions(), "key", &value);  // Status::NotFound() -- data loss on read

Fix

Pass a real is_blob_index flag through in both Get and MultiGet, and when it comes back true, resolve the raw blob reference via the VersionBlobFetcher both methods already construct -- mirroring what Version::Get does after its own TableReader lookup succeeds.

Why the existing test suite never caught this

The original issue's investigation (thank you @rhubner for the deep dive) suspected Options::fs / filesystem-implementation differences, based on CurrentOptions() (the DBTestBase test-framework helper) not reproducing the bug while a bare default-constructed Options() did.

The actual variable was max_open_files: DBTestBase::GetDefaultOptions() hardcodes 5000 (db/db_test_util.cc), while a real default-constructed Options() has RocksDB's actual default of -1 -- exactly the value CompactedDBImpl::Open requires to engage at all. With max_open_files != -1, DB::OpenForReadOnly always falls through to the ordinary DBImplReadOnly path, where this bug does not exist. Since none of the blob tests use RocksDB's parameterized kInfiniteMaxOpenFiles config sweep (the one existing config that does set -1), the CompactedDBImpl fast path was never exercised against blob data by any existing test.

I verified this directly: temporarily forcing GetDefaultOptions() to -1 and running the full db_blob_basic_test suite (46 tests) all still pass with this fix applied, confirming this was an isolated gap rather than a symptom of something broader in CompactedDBImpl. Without the fix, the new regression test below reproducibly fails with the reported NotFound.

Test plan

• Added DBBlobBasicTest.GetBlob_ReadOnlyFullyCompacted to db/blob/db_blob_basic_test.cc, covering both Get and MultiGet through CompactedDBImpl, with max_open_files = -1 explicitly set (the DBTestBase default of 5000 would skip the fast path this test targets).
• Confirmed the new test fails with Status::NotFound() when the fix is reverted, and passes with it applied.
• Full db_blob_basic_test suite (46 tests) passes with the fix.
• make check-sources and make format-auto clean.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 105.3s.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 877fbec

---

Summary

This is a well-motivated, correctly-scoped bug fix for a real data-loss-on-read issue in CompactedDBImpl. The core fix (passing &is_blob_index instead of nullptr and resolving via FetchBlob) is correct and mirrors the Version::Get reference implementation. One medium-severity inconsistency was found in the MultiGet path.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. MultiGet uses lkey.user_key() instead of get_context.ukey_to_get_blob_value() for blob fetch key -- compacted_db_impl_sync_and_async.h:201

• Issue: In the MultiGet blob resolution path, the PR uses lkey.user_key() as the first argument to blob_fetcher.FetchBlob(). The Get path correctly uses get_context.ukey_to_get_blob_value(). The reference implementation in Version::Get (version_set_sync_and_async.h:147) and Version::MultiGet (version_set.cc:2882) both use get_context.ukey_to_get_blob_value().
• Root cause: ukey_to_get_blob_value() returns ukey_with_ts_found_ when user-defined timestamps are enabled and a blob index is found (set at get_context.cc:450-451). This is the user key with the found record's timestamp, which may differ from the lookup timestamp in lkey.user_key(). The blob file stores keys with the record's timestamp, so the lookup must use the matching timestamp.
• Impact: When user-defined timestamps are enabled with blob files on a CompactedDBImpl, MultiGet could pass the wrong timestamp to FetchBlob, potentially causing blob lookup failure. This is a narrow scenario (UDT + blobs + CompactedDBImpl), but it's a correctness issue.
• Suggested fix: Change lkey.user_key() to get_context.ukey_to_get_blob_value() in the MultiGet blob resolution:

  statuses[i] =
      blob_fetcher.FetchBlob(get_context.ukey_to_get_blob_value(), pinnable_val,
                             prefetch_buffer, &blob_value, bytes_read);

🟢 LOW / NIT

L1. Missing IsIncomplete handling for kBlockCacheTier reads -- compacted_db_impl_sync_and_async.h:92-101

• Issue: Version::Get (version_set_sync_and_async.h:151-153) handles FetchBlob returning IsIncomplete() by calling get_context.MarkKeyMayExist() before returning. The PR's fix just propagates the Incomplete status directly. This is a behavioral difference from the reference implementation.
• Impact: Low. CompactedDBImpl is a read-only fast path, and kBlockCacheTier reads are uncommon. Returning Incomplete is still a valid response. However, callers that check value_found (set via MarkKeyMayExist) may behave differently.
• Suggested fix: For consistency with Version::Get, optionally add IsIncomplete handling with MarkKeyMayExist.

L2. Test does not verify CompactedDBImpl was actually instantiated -- db_blob_basic_test.cc:89

• Issue: The test sets max_open_files = -1 and writes a single flushed file, then does ReadOnlyReopen. This should trigger CompactedDBImpl, but the test doesn't assert that. If the conditions for CompactedDBImpl::Open change in the future, this test could silently fall through to DBImplReadOnly and no longer exercise the fix.
• Suggested fix: Consider adding a sync point or property check to confirm CompactedDBImpl is the active implementation.

L3. Test comment block is long relative to the test body -- db_blob_basic_test.cc:66-75

• Issue: The 8-line comment block preceding the test is thorough but relatively long for a regression test. Purely stylistic.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Notes
WritePreparedTxnDB	NO	N/A	CompactedDBImpl is read-only
ReadOnly DB	YES (this is the fix target)	YES	Core fix is correct
User-defined timestamps	YES	GET: YES, MULTIGET: NO (M1)	MultiGet uses wrong key
Merge operator	NO	N/A	CompactedDBImpl::Open rejects merge_operator
WideColumnEntity	Not via CompactedDBImpl	N/A	CompactedDBImpl passes columns=nullptr

Positive Observations

• The fix correctly mirrors the Version::Get blob resolution pattern
• The Get path correctly uses get_context.ukey_to_get_blob_value() for timestamp-aware blob lookup
• Error propagation from FetchBlob is handled correctly
• The VersionBlobFetcher was already constructed but unreachable; the fix simply makes it reachable
• The test correctly forces max_open_files = -1 to ensure CompactedDBImpl is used
• Excellent PR description with thorough root cause analysis
• Release note is appropriately concise

---

ℹ️ 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

[dfa1]

@pdillinger @joshkang97 hello guys, can you please review this code?

So far the bug was not caught by tests because GetDefaultOptions() used in tests has max_file_open=5000 and the bug is triggered only if max_file_open=-1. The fix is really "direct" and I'm not sure it can be improved (Claude review is pointing in a certain direction but I don't fully understand it).