Track newer snapshot versions in RocksDB metadata reads (#15086)

[xingbowang]

Summary:

Add experimental GetWithMetadata and MultiGetWithMetadata APIs that can report whether an explicit-snapshot read observed a newer committed write for the same key while still returning the snapshot-visible result. The implementation widens metadata reads only when the opt-in newer-version field is requested, preserves snapshot visibility with the existing read callback machinery, and forwards the API through the C/C++/Java wrapper surfaces.

This update addresses the latest review feedback by documenting and asserting the range-tombstone lookup-sequence versus snapshot-sequence relationship, avoiding heap allocation for small metadata MultiGet forwarding batches while preserving a contiguous large-batch fallback, and adding coverage for newer point writes and range tombstones in immutable memtables.

Differential Revision: D104182397

[meta-codesync]

@xingbowang has exported this pull request. If you are a Meta employee, you can view the originating Diff in D104182397.

[github-actions]

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

Completed in 1223.0s.

Summary by check

Check	Count
clang-analyzer-core.NullDereference	8
Total	8

Details

db/c.cc (8 warning(s))

db/c.cc:2652:22: warning: Dereference of null pointer (loaded from variable 'timestamp_len') [clang-analyzer-core.NullDereference]
db/c.cc:2653:18: warning: Dereference of null pointer (loaded from variable 'timestamp') [clang-analyzer-core.NullDereference]
db/c.cc:2658:18: warning: Dereference of null pointer (loaded from variable 'timestamp') [clang-analyzer-core.NullDereference]
db/c.cc:2659:22: warning: Dereference of null pointer (loaded from variable 'timestamp_len') [clang-analyzer-core.NullDereference]
db/c.cc:2836:27: warning: Array access (from variable 'timestamp_list') results in a null pointer dereference [clang-analyzer-core.NullDereference]
db/c.cc:2837:33: warning: Array access (from variable 'timestamp_list_sizes') results in a null pointer dereference [clang-analyzer-core.NullDereference]
db/c.cc:2844:27: warning: Array access (from variable 'timestamp_list') results in a null pointer dereference [clang-analyzer-core.NullDereference]
db/c.cc:2845:33: warning: Array access (from variable 'timestamp_list_sizes') results in a null pointer dereference [clang-analyzer-core.NullDereference]

[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 8801143

---

Summary

Large, well-structured PR adding experimental GetWithMetadata()/MultiGetWithMetadata() APIs. The core approach of widening the lookup sequence and using a ReadCallback to filter back to snapshot visibility is sound. Several correctness, performance, and API issues need attention.

High-severity findings (3):

• [db/memtable.cc] Range tombstone metadata detection uses dual-iterator pattern with different sequence numbers for metadata vs masking — complex and fragile, needs stronger documentation and assertions.
• [db/db_impl/db_impl_sync_and_async.h:207] GetWithTimestampReadCallback forced on non-callback paths adds IsVisible() overhead per entry even though the original path had no callback.
• [db/c.cc:2647] C API reads *output_metadata.newer_version_present before status check — relies on callee always initializing the output, fragile for future subclass overrides.

Full review (click to expand)

Findings

🔴 HIGH

H1. Range tombstone dual-iterator complexity — db/memtable.cc

• Issue: When metadata_ctx != nullptr, the code creates TWO range tombstone iterators per memtable: one with the wider seq for metadata detection, and one with the snapshot seq for masking. The "latest" iter checks for covering tombstones between snapshot and upper_bound. The masking iter uses range_del_read_seq = metadata_ctx->read_snapshot_seq. This dual-iterator pattern is correct in isolation but creates subtle ordering dependencies — the metadata iter must run first, and the masking iter must use a seq that doesn't accidentally include tombstones beyond the snapshot. The code appears correct but is fragile and under-documented.
• Root cause: The single-pass widened-lookup design requires separating "what's visible" from "what's newer" for range tombstones.
• Suggested fix: Add assertions: assert(range_del_read_seq <= GetInternalKeySeqno(key.internal_key())). Add a block comment explaining why two iterators are needed and their seq relationship. Consider extracting into a helper function.

H2. ReadCallback forced on non-callback path — db/db_impl/db_impl_sync_and_async.h:207

• Issue: For metadata-tracking reads without timestamps or transactions, GetWithTimestampReadCallback is injected:

  } else if (track_newer_versions && get_impl_options.callback == nullptr) {
      read_cb.Refresh(snapshot);
      get_impl_options.callback = &read_cb;
  }

  This adds per-entry IsVisible() overhead on a path that previously had no callback. The ReadCallback::IsVisible() fast path (seq < min_uncommitted_) handles most entries efficiently, but the callback vtable dispatch is still an added cost on the hot read path when metadata is requested.
• Root cause: Inherent to the widened-lookup design.
• Suggested fix: Document the performance cost. Consider benchmarking metadata reads vs regular reads to quantify the overhead.

H3. C API null-safety on error path — db/c.cc:2647

• Issue: rocksdb_get_with_metadata_impl reads *output_metadata.newer_version_present unconditionally after GetWithMetadata returns, before checking s.ok(). While the current DB::GetWithMetadata and DBImpl::GetWithMetadata implementations always initialize newer_version_present to false before returning errors, a future DB subclass override might not. The C API should not rely on this.
• Suggested fix: Move the newer_version_present read to after the status check, or initialize *newer_version_present = 0 before calling GetWithMetadata.

🟡 MEDIUM

M1. Bloom filter bypass in MultiGet for metadata tracking — db/memtable.cc

• Issue: When track_range_del_metadata is true, the memtable Bloom filter is bypassed for ALL keys in the batch, not just keys needing metadata. This is because the check bloom_filter_ && !apply_range_del && !track_range_del_metadata is batch-wide. For large batches where most keys don't need metadata, this is a significant performance regression.
• Suggested fix: Consider per-key bloom bypass based on whether iter->metadata_ctx != nullptr.

M2. KeyContext struct bloat — table/multiget_context.h

• Issue: Adding newer_version_present (bool) and metadata_ctx (pointer) to KeyContext increases its size. This struct is stack-allocated in arrays of 32 on every MultiGet, even when metadata is not used. The added ~16 bytes per key (with alignment) means ~512 bytes more stack per batch.
• Suggested fix: Acceptable trade-off but document it. Consider a parallel autovector for metadata fields if profiling shows impact.

M3. GetLastPublishedSequence() sampled after SuperVersion — db/db_impl/db_impl_sync_and_async.h

• Issue: The published sequence upper bound is sampled AFTER the SuperVersion is obtained. Concurrent writes between these points produce a sequence number that the SuperVersion's SST files don't contain. Result: potential false negatives (newer version exists but not detected) but no false positives. Acceptable for "hints" but should be documented.
• Suggested fix: Add a comment explaining the false-negative window is acceptable.

M4. StackableDB forwarding not visible in diff — include/rocksdb/utilities/stackable_db.h

• Issue: The diff is truncated but StackableDB must forward GetWithMetadata/MultiGetWithMetadata to db_->*. Without this, wrappers like TTL DB and legacy BlobDB will get NotSupported for all metadata reads with snapshots.
• Suggested fix: Verify forwarding exists.

M5. Duplicate metadata extraction in DB subclass overrides — multiple files

• Issue: The pattern of extracting timestamps/newer_version_present from OutputMetadata/MultiGetOutputMetadata and initializing them is copy-pasted across CompactedDBImpl, DBImplReadOnly, DBImplSecondary. Each has ~15 lines of identical boilerplate.
• Suggested fix: Extract into shared helpers or make the OutputMetadata classes self-initializing.

M6. CompactedDB heap allocation for MultiGetWithMetadata — db/db_impl/compacted_db_impl.cc

• Issue: CompactedDBImpl::MultiGetWithMetadata allocates std::vector<ColumnFamilyHandle*> mutable_column_families(num_keys) on the heap. For small batches, autovector would avoid the allocation.
• Suggested fix: Use autovector<ColumnFamilyHandle*, MultiGetContext::MAX_BATCH_SIZE>.

🟢 LOW / NIT

L1. Test helpers could be shared — db/db_basic_test.cc:37-46

• Issue: NewerVersionOutputMetadata() and NewerVersionMultiGetOutputMetadata() are in an anonymous namespace but duplicated concepts appear in other test files.

L2. Missing test: immutable memtable metadata detection

• Issue: Tests cover active memtable and post-flush SST reads, but no test explicitly verifies metadata detection across immutable memtables (between flush trigger and flush completion).

L3. Missing test: transaction DB rejection

• Issue: No test verifies that WritePreparedTxnDB::GetWithMetadata returns NotSupported when combined with a snapshot.

L4. CopyNewerVersionPresent dual overloads — db/db_impl/db_impl.cc

• Issue: Two overloads exist for bool* and std::vector<bool>*. This follows existing patterns but adds code.

Cross-Component Analysis

Context	Handled?	Notes
WritePreparedTxnDB	Yes	Rejected when callback != nullptr
ReadOnly DB	Yes	Returns false metadata
Secondary DB	Yes	Returns NotSupported with snapshot
CompactedDB	Yes	Returns false metadata
User-defined timestamps	Yes	Tested
Row cache	Yes	Bypassed during tracking
MemPurge	Unknown	MetadataReadCtx passes through imm path; needs verification
BlobDB (integrated)	Likely OK	Direct-write resolution after metadata tracking

Positive Observations

1. Clean opt-in design via OutputMetadata::Want*() methods — extensible and zero-overhead when not used.
2. Correct snapshot isolation — widened lookup + ReadCallback filtering preserves the invariant that returned values match regular Get().
3. Comprehensive tests — 15+ test cases covering diverse scenarios including row cache, range deletes, merges, cross-CF, error handling, and special DB modes.
4. Proper rejection of incompatible modes (kPersistedTier, transaction callbacks).
5. UNLIKELY annotation on the hot-path metadata check in SaveValue.

---

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

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit c1dd050

---

Summary

A well-structured PR adding experimental GetWithMetadata/MultiGetWithMetadata APIs with opt-in newer-version tracking. The widened-lookup-key + ReadCallback-filtering approach is sound and preserves snapshot semantics. The implementation is thorough across DB subclasses (ReadOnly, Secondary, Compacted, WritePrepared, BlobDB, TTL) and wrapper surfaces (C, Java). Test coverage is comprehensive for the primary code paths.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. GetLastPublishedSequence() called after SV pin may see sequences not in the pinned SV — db/db_impl/db_impl_sync_and_async.h

• Issue: newer_version_upper_bound_seq = GetLastPublishedSequence() is called after the SuperVersion is pinned. A write that publishes a new sequence number into a new memtable (after an SV switch) between SV pin and this call creates an upper bound that references entries NOT present in the pinned SV. These entries will never be found during the lookup, producing a false negative.
• Root cause: The window between SV pin and sequence sampling is inherently racy.
• Assessment: This is documented as a best-effort caveat ("false negatives are possible because the latest visible sequence is sampled after the SuperVersion is pinned"). The documentation in include/rocksdb/db.h correctly describes this. This is a design decision, not a bug, but worth noting for completeness.
• Suggested fix: None needed — the documentation is accurate. Consider sampling GetLastPublishedSequence() before SV pin (accepting a slightly different tradeoff) if false negatives prove problematic in practice.

M2. Bloom filter disabled for entire memtable batch when any key needs range tombstone metadata — db/memtable.cc:1793

• Issue: When track_range_del_metadata is true (any key in the batch hasn't yet observed a newer version AND range tombstones exist), the bloom filter optimization is disabled for the entire MultiGet batch, not just the keys needing metadata. This could cause a noticeable performance regression for metadata MultiGet reads in memtables with many keys.
• Root cause: The bloom filter check happens before per-key processing, and the range tombstone metadata tracking needs to see all keys regardless of bloom filter results.
• Suggested fix: Consider a two-pass approach: first pass with bloom filter for non-metadata keys, second pass for metadata keys. However, this adds complexity and the current approach is correct. The performance impact is limited to metadata reads only (opt-in), which mitigates the concern.

M3. std::vector<bool> used for newer_version_present output — include/rocksdb/db.h, db/db_impl/db_impl.cc

• Issue: std::vector<bool> is bit-packed and has well-known issues: operator[] returns a proxy reference (not a real bool&), iteration is slower than vector<char>, and it's not thread-safe for concurrent bit access. The MetadataReadCtx holds a bool& which binds to KeyContext::newer_version_present (a real bool field), not to vector<bool> elements, so the reference binding is safe. However, the output vector<bool> is populated via assign() and indexed with [], which is fine but non-idiomatic.
• Root cause: std::vector<bool> is a specialization that doesn't behave like other vectors.
• Suggested fix: Consider using std::vector<uint8_t> or keeping std::vector<bool> with a comment acknowledging the specialization. Since the final copy happens sequentially in CopyNewerVersionPresent, this is functionally correct.

M4. C API rocksdb_get_with_metadata_impl accesses newer_version_present before checking s.ok() — db/c.cc:2646

• Issue: After calling GetWithMetadata, the code unconditionally accesses *output_metadata.newer_version_present when newer_version_present != nullptr, even if the status is an error (not ok and not NotFound). While GetWithMetadata guarantees initialization of the field to false, and SaveError is called for non-NotFound errors, this pattern differs from how the existing rocksdb_get handles errors.
• Root cause: The newer_version_present field is always initialized by the C++ API, so this is safe but could be confusing.
• Suggested fix: Consider moving the newer_version_present read inside the status-check branches for consistency, though current behavior is correct.

🟢 LOW / NIT

L1. Duplicated metadata extraction pattern across DB subclasses — multiple files

• Issue: The pattern of extracting timestamps/newer_version_present from MultiGetOutputMetadata and initializing them is duplicated in CompactedDBImpl, DBImplReadOnly, DBImplSecondary, BlobDBImpl, and DBWithTTLImpl. Each has ~15 lines of identical boilerplate.
• Suggested fix: Extract a shared helper function (e.g., PrepareMultiGetOutputMetadata) to reduce duplication. The single-key OutputMetadata extraction already has helpers (GetOutputTimestamp, GetOutputNewerVersionPresent) in an anonymous namespace in db_impl.cc, but they're not shared.

L2. Stack/heap allocation strategy inconsistency in DB subclass MultiGetWithMetadata — compacted_db_impl.cc, db_impl_readonly.cc, db_impl_secondary.cc

• Issue: The CompactedDBImpl, DBImplReadOnly, and DBImplSecondary MultiGetWithMetadata overrides use autovector + std::vector fallback for mutable_column_families, while DBWithTTLImpl uses std::array + std::unique_ptr<[]>. The TTL version uses std::array<ColumnFamilyHandle*, MAX_BATCH_SIZE> which is stack-allocated but not auto-sized.
• Suggested fix: Standardize on one pattern. The autovector approach used by the core DB subclasses is preferred per RocksDB conventions.

L3. GetWithTimestampReadCallback name is misleading when used for non-timestamp metadata tracking — db/db_impl/db_impl_sync_and_async.h:209

• Issue: When track_newer_versions && callback == nullptr (non-timestamp CF), a GetWithTimestampReadCallback is installed purely as a visibility filter. The callback's IsVisible method (seq <= max_visible_seq_) works correctly regardless of timestamps, but the class name suggests timestamp-specific behavior. The IsNewerVisibleForMetadataRead override added to this class is correctly generalized.
• Suggested fix: A comment at the installation site noting this reuse would improve clarity. This is minor since the code is correct.

L4. Java JNI getWithMetadata always requests both timestamp and newer_version_present — java/rocksjni/rocksjni.cc

• Issue: The JNI implementation always calls output_metadata.WantTimestamp().WantNewerVersionPresent(), even when the caller may not need the timestamp. This forces unnecessary work when only newer_version_present is needed. The GetWithMetadataResult Java class always includes the timestamp field.
• Suggested fix: Consider adding separate Java API methods or a flags parameter to control which metadata is requested. This is acceptable for an experimental API.

L5. Missing rocksdb_multi_get_cf_with_metadata declaration check — include/rocksdb/c.h

• Issue: The rocksdb_multi_get_cf_with_metadata C API function takes const rocksdb_column_family_handle_t* const* for the column_families parameter, which is more const-qualified than rocksdb_multi_get_cf which takes const rocksdb_column_family_handle_t* const*. This is consistent but differs from how some other C API functions handle column families (some use non-const).
• Suggested fix: This is fine as-is; the const-correctness is appropriate.

Cross-Component Analysis

Context	Behavior	Assessment
WritePreparedTxnDB	Delegates to DB::GetWithMetadata base → falls back to Get() without tracking when snapshot is set	Correct: custom read callbacks are unsupported, so tracking is skipped
ReadOnly DB	Sets newer_version_present = nullptr in GetImpl, returns false for MultiGet	Correct: no writes possible, so no newer versions
Secondary DB	Returns NotSupported for snapshot reads	Correct: secondary has limited write visibility guarantees
CompactedDB	Always returns false	Correct: no new writes can occur
BlobDB	Forwards newer_version_present through GetImpl	Correct: blob index resolution happens after metadata tracking
TTL DB	Delegates to base class, strips TTL timestamp	Correct: metadata tracking is independent of TTL value format
Row cache	Bypassed when tracking active	Correct: cached rows don't expose sequence numbers
User-defined timestamps	GetWithTimestampReadCallback::IsNewerVisibleForMetadataRead handles correctly	Correct: checks snapshot < seq <= max_visible_seq
kPersistedTier	Returns NotSupported	Correct: cannot guarantee seeing all relevant sequences

Assumption stress test:

1. Claim: "snapshot visibility is preserved" — Verified. The LookupKey uses lookup_snapshot (wider) to scan more entries, but ReadCallback::IsVisible(seq) still filters at snapshot (original). Range tombstone masking uses range_del_read_seq (= original snapshot). The max_covering_tombstone_seq is set by the original-snapshot iterator. Entry selection and merge context accumulation respect the original snapshot.

2. Claim: "false negatives are possible" — Verified. The window between SV pin and GetLastPublishedSequence() call can miss concurrent writes. Additionally, if consistent_seqnum == newer_version_upper_bound_seq, tracking is skipped entirely (track_newer_versions = false), so writes published exactly at the snapshot boundary are missed.

3. Claim: "row cache bypass is necessary" — Verified. Cached rows store values without sequence numbers. A row cache hit would skip the memtable/SST scan where metadata tracking occurs. After a newer version is observed, subsequent lookups CAN use the row cache (the NeedToTrackNewerVersions() check is false when HasNewerVersion() returns true).

Positive Observations

• The opt-in OutputMetadata design with std::optional fields ensures zero overhead for callers that don't request metadata.
• The MetadataReadCtx design cleanly separates metadata tracking state from the main read path, using const pointers and a reference to the output bool.
• The test suite is thorough, covering immutable memtables, range tombstones with ignore_range_deletions, row cache bypass, read-only DB, compacted DB, cross-CF MultiGet, kPersistedTier rejection, and null-value validation.
• The db_stress integration validates consistency of newer-version metadata across batched operations.
• The db_bench integration with --read_with_metadata enables performance benchmarking.
• The assertion assert(range_del_read_seq <= lookup_seq) in memtable.cc provides a safety net for the two-iterator design.
• The single shared latest_range_del_iter per MultiGet batch (rather than per-key) is a good optimization.

---

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