[rban1]

Summary:

X-link: #15203

Adds experimental DB::ApplyLsmEdit() for installing caller-declared SST additions, deletions and levels atomically across column families. Unlike IngestExternalFile(), which chooses placement, this API validates the requested LSM layout. It lets callers transplant a prepared layout or replace data without first compacting destination files merely to move their boundaries.

File changes and placement

• Explicit mode — kExplicitFiles: removes only the listed live files. Unlisted files remain, even inside the edit’s range. Supports additive imports, deletion-only edits and file replacement.
• Range mode — kFilesInRange: removes files wholly contained in [start, limit). Boundary-straddling files must retain their out-of-range data. Preserved-sequence imports probe them across all levels and reject the edit if they contain points or covering tombstones inside the range. Eligible external-V2 sequence-zero replacements can instead receive fresh sequences and a covering tombstone.
• Levels are normally exact: L1+ file boundaries must remain non-overlapping, including against surviving files. Probing does not relax this structural rule. Optional relocation applies only to eligible sequence-zero range replacements and preserves relative run order.
• Optional merging: merge_incoming_files rewrites incoming files into one SST per declared level using destination settings. merge_existing_files additionally incorporates overlapping same-level destination files, preserving their out-of-range contents. It requires an add-only explicit edit and can rewrite substantially more data than the incoming files.

Unset range endpoints mean the whole CF; otherwise both endpoints are required. The range bounds incoming and explicitly deleted files, but destination-merge output may extend beyond it.

Sequences, snapshots and safety

• Preserved-sequence imports retain their entry sequences. The destination sequence watermark advances when necessary so subsequent writes are newer.
• Overlapping histories are validated against added and surviving files. Supported sequence-zero point ties follow declared level/L0 run priority consistently through reads and compaction; range tombstones must still be strictly newer than covered points.
• zero_sequence_numbers explicitly normalizes point snapshot inputs during merging, while non-reset online changes retain their sequences. Equal-key/sequence merge operands must be consolidated; unsupported collisions return NotSupported.
• Active snapshots are rejected by default. Opt-out permits additive explicit edits or bounded, merged, probed range imports that delete no destination files and verify an empty destination range. It does not generally make foreign-sequence imports snapshot-consistent.
• Installation revalidates current state, prevents conflicting compactions, flushes overlapping memtables or fails when blocking flushes are disabled, and commits atomically. Cleanup preserves source ownership and generated-file accounting. Recent fixes also protect picker bookkeeping, TTL metadata and snapshot-taking I/O callbacks.

Read-only/secondary DBs and timestamp-enabled CFs reject the API; FIFO permits only L0. No SST/MANIFEST format changes are introduced.

October 29 static-analysis cleanup: explicitly delete move construction/assignment for ScopedDeferredFileIOListeners, FileIOCallbackGuard and OwnedFileOperationInfo, and default the owned-event destructor. These types remain noncopyable/nonmovable, preserving registered object identity and borrowed references into owned event data. Reserve the exact input-level vector capacity before appending. This addresses all four clang-tidy comments without suppressions or API/format/sequence changes.

October 29 third-review concurrency fixes (F001-F003): deletion reservation cleanup clears its bookkeeping after the protected picker refresh, so later job destruction does not access current-Version or mutable-CF state. LSM edit callback scopes sharing configured listeners now hold an exclusive lease through I/O callback delivery, before another matching edit can enter DB/write locks. Non-overlapping listener sets may proceed independently. Matching reentrant LSM edits return NotSupported before preparation or commit; this also prevents recursive MANIFEST writes during file-number reservation. Direct and deferred callbacks are tracked with a thread-local callback stack. File contents, level/sequence rules and SST/MANIFEST formats are unchanged. This update is confined to the base draft; downstream release refresh and MyRocks testing are deferred at the user's request.

Strict checked-status validation also exposed two existing experimental paths that could abort: a default Status was unused in an empty deletion-only batch, and BuildMergedFile() left BuildTable()'s secondary I/O status unread on an early iterator failure. Scope the per-file status inside its loop, and consume the I/O status while preserving the primary error (propagating an I/O error if the primary status is OK). No checks are suppressed.

Differential Revision: D119743760

[meta-codesync]

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

[github-actions]

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

Completed in 607.6s.

Summary by check

Check	Count
cppcoreguidelines-avoid-non-const-global-variables	1
cppcoreguidelines-special-member-functions	1
modernize-use-emplace	1
Total	3

Details

db/lsm_edit_test.cc (2 warning(s))

db/lsm_edit_test.cc:109:14: warning: use emplace_back instead of push_back [modernize-use-emplace]
db/lsm_edit_test.cc:176:7: warning: class 'LsmEditTest' defines a non-default destructor but does not define a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

file/file_util.cc (1 warning(s))

file/file_util.cc:38:41: warning: variable 'current_io_callback' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

[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 6b49c53

---

Summary

Well-structured addition of DB::ApplyLsmEdit() that cleanly reuses the existing ExternalSstFileIngestionJob infrastructure. The design is sound -- declared placement and straddler probing solve real problems (unnecessary compaction rewrites) and the implementation correctly handles the key correctness requirements (range emptiness verification, level overlap checking with doomed-file awareness, sequence number preservation). Code quality is high with good test coverage of the main scenarios.

High-severity findings (1):

• [external_sst_file_ingestion_job.cc: DeclaredFileFitsInLevel] Linear scan over all level files is O(N) per ingested file vs the O(log N) binary search used by IngestedFileFitInLevel/OverlapInLevel. On large levels (L6 with thousands of files), this can be costly when many files are being installed. This is a performance concern, not a correctness bug, but it is on a mutex-held path.

Full review (click to expand)

Findings

🔴 HIGH

H1. DeclaredFileFitsInLevel uses linear scan instead of binary search -- external_sst_file_ingestion_job.cc

• Issue: DeclaredFileFitsInLevel iterates ALL files at a given level to check for overlap, while the existing IngestedFileFitInLevel delegates to vstorage->OverlapInLevel() which uses SomeFileOverlapsRange with binary search for sorted levels (L1+). For large levels (e.g., L6 with thousands of files), this is O(N) per ingested file vs O(log N). Worse, this runs while the DB mutex is held (REQUIRES: Mutex held), so the linear scan holds the mutex longer than necessary.
• Root cause: The method needs to skip files pending deletion in edit_.GetDeletedFiles(), which the existing OverlapInLevel does not support. A linear scan was the simplest implementation.
• Suggested fix: Consider building a temporary set of non-deleted files or using OverlapInLevel and then checking if the overlapping file is in the deleted set. Alternatively, for levels where edit_.GetDeletedFiles() has no entries for this level, fall through to OverlapInLevel directly. This is acceptable for an EXPERIMENTAL API but should be addressed before stabilization.

🟡 MEDIUM

M1. No validation of move_files / link_files mutual exclusion in ApplyLsmEdit -- db_impl.cc

• Issue: LsmEditOptions exposes both move_files and link_files with documentation saying "Cannot be combined". The validation happens downstream in PrepareFileIngestionImpl (via the IngestExternalFileOptions check at line 6732), so the error IS caught. However, the error message says `move_files` and `link_files` can not both be true which references the internal IngestExternalFileOptions field names, not the LsmEditOptions field names. This is a minor UX issue -- the validation works but the error message could confuse users of the new API.
• Suggested fix: Add an early check in ApplyLsmEdit() itself, before building the internal args, with an error message referencing LsmEditOptions.

M2. No stress test (db_stress) coverage -- db_stress_tool/

• Issue: The CLAUDE.md guidelines state "When adding a new feature, make sure stress test covers the new option." No db_stress coverage was found for ApplyLsmEdit. This is important for catching concurrency bugs that unit tests miss.
• Suggested fix: Add ApplyLsmEdit support to db_stress, exercising concurrent edits alongside reads/writes/compactions.

M3. No db_bench support -- tools/

• Issue: CLAUDE.md guidelines state "When adding a performance related feature, support it in db_bench." No db_bench benchmark was added. The straddler probing feature has performance characteristics (seek cost per straddling file) that should be benchmarkable.
• Suggested fix: Add a db_bench mode for ApplyLsmEdit to measure ingestion latency with varying numbers of straddling files.

M4. Potential snapshot visibility anomaly with foreign sequence numbers -- db_impl.cc:7058-7063

• Issue: When files from another DB are installed, their sequence numbers may be from a completely different sequence space. The commit path advances LastSequence to cover them (versions_->SetLastSequence(max_assigned_seqno)). If the foreign seqnos are large, this creates a gap in the sequence number space. A snapshot taken during this gap would be assigned a seqno in the gap, which correctly makes the grafted data visible. However, if the foreign seqnos happen to collide with existing seqnos of data OUTSIDE the edit range, both pieces of data are visible to all snapshots, which is correct. The concern is: if a snapshot was taken BEFORE the edit with a seqno that happens to be between the foreign file's smallest and largest seqno, the snapshot would see a partial view of the grafted data. This is semantically correct (the snapshot predates the edit) but may surprise callers who expect the grafted data to be all-or-nothing visible.
• Root cause: Foreign sequence numbers from another DB instance are not in the same logical sequence space.
• Suggested fix: Document this behavior explicitly in the API documentation. Consider whether ApplyLsmEdit should reject edits when outstanding snapshots exist, or at least document that callers should manage snapshot lifecycle around edits.

M5. FIFO compaction not validated -- external_sst_file_ingestion_job.cc

• Issue: FIFO compaction only meaningfully uses level 0. If ApplyLsmEdit declares files at levels > 0 with FIFO compaction style, RecordDeclaredLevels accepts them (level is within [0, num_levels)) but the files would be placed at levels that FIFO compaction never compacts. This could lead to data that never gets cleaned up.
• Suggested fix: Either reject multi-level placement with FIFO compaction, or document the limitation.

🟢 LOW / NIT

L1. RecordDeclaredLevels creates std::map for level grouping -- external_sst_file_ingestion_job.cc

• Issue: Uses std::map<int, autovector<const IngestedFileInfo*>> for grouping files by level. Given the small number of levels (typically 7), a flat vector or even a fixed-size array would be more cache-friendly and avoid heap allocations. This is not on a hot path (called once during Prepare), so the impact is negligible.
• Suggested fix: Consider using a flat structure, but this is purely a nit given the cold-path nature.

L2. ApplyLsmEdit does not validate column_family != nullptr early -- db_impl.cc

• Issue: If a caller passes nullptr for column_family in an LsmEdit, it propagates to PrepareFileIngestionImpl where it's caught. The error path works but the error message ("column family handle is null") is generic. An earlier check in ApplyLsmEdit could give a more specific message.
• Suggested fix: Add if (edit.column_family == nullptr) return Status::InvalidArgument("LSM edit has null column family") in the validation loop.

L3. Adding pure virtual method to DB class -- include/rocksdb/db.h

• Issue: Adding virtual Status ApplyLsmEdit(...) = 0 is an ABI-breaking change for out-of-tree DB subclasses. Appropriately marked as EXPERIMENTAL, matching PrepareFileIngestion() precedent.
• Suggested fix: Already acceptable. All required overrides are present (DBImpl, StackableDB, ModelDB).

L4. Test coverage gaps (non-blocking)

• Issue: No tests for: memtable overlap triggering flush, multiple straddling files at different levels, concurrent ApplyLsmEdit and compaction, FIFO compaction style. The 15 tests cover the main scenarios well for an EXPERIMENTAL API.
• Suggested fix: Consider adding these before stabilization.

Cross-Component Analysis

Context	Executes?	Assumptions hold?	Notes
WritePreparedTxnDB	Yes (StackableDB delegates)	Yes	Foreign seqnos not in prepare map, so visible
ReadOnly DB	No (ModelDB returns NotSupported)	N/A	Safe
User-defined timestamps	No (RecordDeclaredLevels rejects)	N/A	Correctly blocked
FIFO compaction	Yes	Partially	Multi-level placement accepted but may not compact (M5)
Universal compaction	Yes	Yes	Level semantics still apply
BlobDB	Yes (StackableDB)	Yes	External files are standard SSTs
Concurrent writes	Yes	Yes	NeedsFlush + allow_blocking_flush handle memtable overlap
MemPurge	Yes	Yes	MemPurge is a flush variant; NeedsFlush still works
Snapshots	Yes	Mostly	See M4 about foreign seqno visibility

Positive Observations

• Clean reuse of existing infrastructure: The ExternalSstFileIngestionJob reuse avoids duplicating the complex prepare/commit/flush/rollback logic. The LsmEditJobSpec optional cleanly separates the new behavior.
• Thorough validation: RecordDeclaredLevels checks level bounds, UDT incompatibility, and same-level overlap. ProbeStraddlingFiles checks both point keys and range tombstones.
• Good test coverage: 15 tests covering multi-level, multi-CF, rollback, persistence, and various rejection cases.
• Correct sequence number handling: The allow_db_generated_files = true + allow_global_seqno = false combination correctly preserves original seqnos without modifying the files.
• DeclaredFileFitsInLevel correctly accounts for doomed files: The edit's deleted files are excluded from overlap checking, allowing a declared level to reuse space freed by the edit.
• Range tombstone detection in ProbeStraddlingFiles: The ReadRangeDelAggregator + IsRangeOverlapped check correctly catches range tombstones that would shadow grafted data.

---

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

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

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 1b42be3

---

Summary

Well-structured PR that adds a useful declarative LSM edit primitive by reusing the existing file ingestion infrastructure. The design is sound and the implementation follows existing RocksDB patterns. Most validation paths are well-covered by tests.

High-severity findings (1):

• [external_sst_file_ingestion_job.cc: DeclaredFileFitsInLevel] The overlap check in DeclaredFileFitsInLevel uses direct user-key comparisons with inclusive boundaries (>=/<=), diverging from the standard IngestedFileFitInLevel which delegates to VersionStorageInfo::OverlapInLevel. This inconsistency could cause valid placements to be incorrectly rejected (or vice versa) at boundary keys.

Full review (click to expand)

Findings

🔴 HIGH

H1. DeclaredFileFitsInLevel overlap check uses different boundary semantics than IngestedFileFitInLevel — external_sst_file_ingestion_job.cc

• Issue: DeclaredFileFitsInLevel compares user keys directly:

  if (ucmp_->Compare(limit, existing->smallest.user_key()) >= 0 &&
      ucmp_->Compare(start, existing->largest.user_key()) <= 0) {
    return false;
  }

  This treats both boundaries as inclusive. The existing IngestedFileFitInLevel uses VersionStorageInfo::OverlapInLevel, which has more nuanced boundary handling. The new function also iterates all files at the level linearly (O(n)) instead of using binary search (which OverlapInLevel does for sorted levels > 0).

  The key concern: when an ingested file's largest user key equals an existing file's smallest user key, the >= makes this an overlap and rejects the placement. Whether this is the correct behavior depends on RocksDB's file boundary semantics — files at the same level may share a boundary user key if sequence numbers disambiguate, but DeclaredFileFitsInLevel cannot distinguish this case since it only compares user keys.

• Root cause: New function reimplements overlap logic rather than delegating to OverlapInLevel (or at least matching its semantics). The reason it can't directly use OverlapInLevel is that it needs to skip files being deleted by the same edit, but the overlap check semantics should still match.

• Suggested fix: Either (a) delegate to OverlapInLevel then filter out deleted files from the result, or (b) match OverlapInLevel's exact comparison semantics while adding the deleted-file skip. At minimum, document why the inclusive boundary treatment is correct or intentionally conservative for this use case.

🟡 MEDIUM

M1. RecordDeclaredLevels called after file copy/link — waste of I/O on invalid requests

• Issue: In Prepare(), RecordDeclaredLevels is called after files have already been copied/linked into the DB directory. If validation fails (bad level, overlapping files at same level, UDT column family), the files are cleaned up but the I/O was wasted.

• Suggested fix: Move level validation earlier — either before the copy loop or split it into a pre-validation step that only needs file metadata (which is populated before the copy loop).

M2. L0 file ordering depends on epoch-number assignment order, not seqnos

• Issue: The API doc says "later entries in add_files are treated as newer" for L0. This works because epoch numbers are assigned in array order in Run(). But the files' internal sequence numbers may not match this ordering — a "newer" file (later in array) could have lower seqnos. This is safe because L0 ordering uses epoch numbers, not seqnos, for disambiguating reads. But the implicit dependency on epoch-number assignment order should be documented.

• Suggested fix: Add a comment where epoch numbers are assigned explaining the L0 ordering contract.

M3. No test for memtable-overlap / flush behavior

• Issue: When the edit range has unflushed memtable data, the existing NeedsFlush + allow_blocking_flush machinery handles it. But no test exercises this path. A test that writes to the memtable in the edit range and then calls ApplyLsmEdit would catch regressions in the flush-before-edit path.

M4. ProbeStraddlingFiles — no test for multiple straddling files across levels

• Issue: The test suite has one test for a single straddling file at one level. A test with straddling files at multiple levels would improve confidence.

🟢 LOW / NIT

L1. std::map for level grouping in RecordDeclaredLevels

• Issue: std::map<int, autovector<...>> for grouping files by level. A std::vector indexed by level would be simpler for the bounded number of levels.

L2. DeclaredFileFitsInLevel linear scan on large levels

• Issue: Iterates all files at a level. For L6 with thousands of files, this is O(n). IngestedFileFitInLevel uses OverlapInLevel which does binary search for sorted levels.

L3. Missing test for move_files=true behavior

• Issue: GraftLsmBuiltInAnotherDb tests link_files=true but move_files=true is untested.

L4. Build system registration is complete

• The test is registered in all four build systems (Makefile, CMakeLists.txt, src.mk, BUCK). Correct.

Cross-Component Analysis

Context	Executes?	Assumptions hold?	Notes
WritePreparedTxnDB	Yes (StackableDB passthrough)	Yes	Range verified empty, no conflict with prepared txns
User-defined timestamps	Rejected in RecordDeclaredLevels	N/A	Correctly guarded
Old snapshots	Yes	Yes	Range is empty pre-edit; edit is atomic MANIFEST write
Concurrent compaction	Yes	Yes	Range overlap with compaction checked in Run()
Concurrent writes	Protected	Yes	Write thread entered during commit
FIFO compaction	Yes	Yes	Level 0 only; declared levels validated

Positive Observations

• Clean reuse of existing infrastructure: Extends ExternalSstFileIngestionJob with minimal mode-switch code rather than a parallel path.
• Comprehensive test suite: 13 test cases covering the main use case, error paths, multi-CF atomicity, rollback, L0 ordering, reopen persistence, and cross-DB grafting.
• Elegant straddler probing: One seek per straddling file is an excellent design — avoids requiring file-boundary-aligned ranges.
• Good error messages: Specific, actionable error strings throughout.
• Clean API: Well-separated LsmEditOptions / LsmEdit / LsmEditFile structs are extensible and consistent with existing RocksDB patterns.

---

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

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

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 1666032

---

Summary

Well-designed API that cleanly reuses the existing ExternalSstFileIngestionJob infrastructure. The straddler probing and sequence-order validation are thorough. Two notable issues: DBImplReadOnly and DBImplSecondary do not override ApplyLsmEdit, and DeclaredFileFitsInLevel uses linear scan instead of binary search.

High-severity findings (1):

• [db_impl_readonly.h / db_impl_secondary.h] ApplyLsmEdit is not overridden in DBImplReadOnly or DBImplSecondary -- calls fall through to DBImpl::ApplyLsmEdit and attempt to modify a read-only/secondary instance.

Full review (click to expand)

Findings

🔴 HIGH

H1. Missing ApplyLsmEdit override in DBImplReadOnly and DBImplSecondary -- db_impl_readonly.h, db_impl_secondary.h

• Issue: DB::ApplyLsmEdit() is pure virtual and implemented in DBImpl. DBImplReadOnly and DBImplSecondary both inherit from DBImpl. They override IngestExternalFile and PrepareFileIngestion to return Status::NotSupported, but they do not override ApplyLsmEdit. A call to ApplyLsmEdit on a read-only or secondary DB instance falls through to DBImpl::ApplyLsmEdit, which will attempt to modify the DB (acquire mutexes, write MANIFEST, etc.). This is inconsistent with how the other mutation APIs are handled and could cause data corruption or crashes on read-only instances.
• Root cause: The new virtual method was added to the base class and implemented in DBImpl, but the read-only/secondary subclasses were not updated.
• Suggested fix: Add overrides in both DBImplReadOnly and DBImplSecondary:

  using DB::ApplyLsmEdit;
  Status ApplyLsmEdit(const LsmEditOptions& /*options*/,
                      const std::vector<LsmEdit>& /*edits*/) override {
    return Status::NotSupported("Not supported operation in read only mode.");
  }

🟡 MEDIUM

M1. DeclaredFileFitsInLevel uses linear scan instead of binary search -- external_sst_file_ingestion_job.cc

• Issue: DeclaredFileFitsInLevel iterates over ALL files at the target level to check for overlap. For levels > 0, files are sorted by key range, so VersionStorageInfo::OverlapInLevel (which uses SomeFileOverlapsRange with binary search) would be more efficient. The existing IngestedFileFitInLevel already uses OverlapInLevel. However, DeclaredFileFitsInLevel needs to skip deleted files, which OverlapInLevel cannot do. This is correct but slower for large levels.
• Root cause: The need to skip files in edit_.GetDeletedFiles() prevents reuse of the optimized OverlapInLevel.
• Suggested fix: Consider filtering the deleted files from the level's file list before checking overlap, or accept the linear cost since this runs under the mutex and the number of straddling/deleted files is typically small. Worth a comment explaining the choice.

M2. ValidateDeclaredLevelSequenceOrder is O(n^2) in file count -- external_sst_file_ingestion_job.cc

• Issue: The pairwise validation loop (for i, for j > i) opens table readers and scans point/range data for every overlapping pair. For a large LSM transplant with many overlapping L0 files, this could be expensive. Each pair may open two table readers and perform merge-sort-like iteration.
• Root cause: The validation must compare every pair of overlapping files at different levels (or same L0 level) to verify sequence ordering.
• Suggested fix: The O(n^2) approach is likely acceptable for typical use cases (a handful of files), but a comment noting the quadratic cost and the assumption that file counts are small would be helpful. For very large grafts, an alternative approach (sort all entries by key, verify ordering in a single pass) could be considered.

M3. No stress test or db_stress coverage for ApplyLsmEdit -- missing

• Issue: The CLAUDE.md guidelines state "When adding a new feature, make sure stress test covers the new option." There is no db_stress integration for ApplyLsmEdit. While the unit tests are thorough, stress testing would validate thread safety and interaction with concurrent operations (compactions, flushes, reads).
• Suggested fix: Add ApplyLsmEdit coverage to db_stress or at minimum note it as a TODO.

M4. No db_bench support for the new feature -- missing

• Issue: CLAUDE.md states "When adding a performance related feature, support it in db_bench." While this is more of a correctness feature than a performance feature, benchmarking the probe cost and multi-file graft overhead would be valuable.
• Suggested fix: Consider adding a db_bench scenario or note it as a TODO.

🟢 LOW / NIT

L1. MakeFile helper creates files with SstFileWriter which uses seqno 0 -- lsm_edit_test.cc

• Issue: The MakeFile test helper uses SstFileWriter, which writes keys with sequence number 0. This means several tests (like PreservesLevelZeroOrdering) rely on same-seqno behavior at L0 rather than testing the more realistic case where L0 files have different sequence numbers. The tests RejectsOlderSequenceNumberInHigherLevel and GraftLsmBuiltInAnotherDb correctly use a source DB to produce files with real sequence numbers, covering the non-trivial case.
• Suggested fix: No action needed -- the seqno=0 tests are complementary to the source-DB tests.

L2. No test for concurrent compaction overlap -- lsm_edit_test.cc

• Issue: The Run() method checks RangeOverlapWithCompaction and returns InvalidArgument. There is no test exercising this code path.
• Suggested fix: Add a test that starts a compaction overlapping the edit range and verifies ApplyLsmEdit fails gracefully.

L3. No test for move_files = true -- lsm_edit_test.cc

• Issue: Tests use default (copy) and link_files = true, but move_files = true is not tested.
• Suggested fix: Add a test case.

L4. ProbeStraddlingFiles TODO comment -- external_sst_file_ingestion_job.cc

• Issue: The ProbeStraddlingFiles method has a // TODO: plumb Env::IOActivity, Env::IOPriority comment, which is consistent with other similar TODOs in the codebase.
• Suggested fix: No action needed -- this is a pre-existing pattern.

L5. Iterator correctness not tested after edit -- lsm_edit_test.cc

• Issue: Tests verify correctness with Get() but not with iterators (Seek, Next, Prev). Since ApplyLsmEdit installs files at declared levels and may leave straddling files at the same level, iterator behavior across the graft boundary is worth testing.
• Suggested fix: Add an iterator-based verification to at least one test case.

L6. LsmEditFile struct has int level = 0 as default -- lsm_edit.h

• Issue: Defaulting to level 0 is reasonable but could lead to accidental L0 placement if a caller forgets to set the level. However, the struct is clearly documented and the default matches the most common starting point.
• Suggested fix: No action needed.

L7. checksums_supplied determination from first file -- db_impl.cc

• Issue: The logic const bool checksums_supplied = !edit.add_files.front().checksum.empty() || !edit.add_files.front().checksum_func_name.empty(); determines checksum mode from the first file. If the first file has no checksums but subsequent files do, the mismatch is caught by the later validation loop. However, the || means if only checksum_func_name is set on the first file (but not checksum), checksums_supplied would be true, and then the per-file validation has_checksum != has_checksum_func_name would catch the incomplete first file. This is correct but slightly subtle.
• Suggested fix: A comment clarifying the intent would help readability.

Cross-Component Analysis

Context	Applicable?	Safe?	Notes
ReadOnly DB	YES	NO	Missing override -- H1
Secondary Instance	YES	NO	Missing override -- H1
WritePreparedTxnDB	YES	YES	Inherits from DBImpl, uses allow_db_generated_files path which preserves original seqnos. read_callback_ is not involved since there's no write/read during ingestion.
User-defined timestamps	Blocked	YES	RecordDeclaredLevels explicitly returns NotSupported for timestamp_size > 0.
MemPurge	NO	N/A	ApplyLsmEdit triggers a flush if needed, doesn't interact with MemPurge.
BlobDB	Not tested	Unknown	No validation that ingested files don't reference blob files. Likely safe since external SSTs don't use blob references, but not tested.
FIFO compaction	YES	YES	Declaring a level >= num_levels fails with "outside [0, N)". FIFO typically has num_levels=1, so only L0 is valid.
Universal compaction	YES	YES	Works correctly; the range tombstone fallback is correctly disabled.
Level compaction	YES	YES	Primary tested configuration.

Positive Observations

• Clean reuse of existing infrastructure: Rather than creating a parallel code path, the PR adds an optional LsmEditJobSpec to ExternalSstFileIngestionJob. This minimizes code duplication and ensures that the existing validation, file management, and commit logic is shared.

• Thorough sequence number validation: The LsmEditSequenceOrderValidator checks point entries, range tombstones against points (both directions), and range tombstones against each other. This is comprehensive.

• Well-designed straddler probing: The ProbeStraddlingFiles method elegantly handles the case where a file straddles the edit range boundary. By seeking into the file and checking for live keys, it avoids unnecessary compaction while maintaining correctness.

• Good test coverage: 20 test cases covering the happy path, various rejection scenarios, multi-CF atomicity, persistence, and the full LSM transplant use case.

• Clean API design: The LsmEditOptions / LsmEdit / LsmEditFile structs are well-documented and follow existing RocksDB conventions. The EXPERIMENTAL marking is clear and consistent with PrepareFileIngestion().

• Correct epoch number ordering: Each L0 file gets a NewEpochNumber() in the order they appear in the ingestion loop, which processes files in add_files order (oldest first). This ensures the newest file gets the highest epoch number, preserving the intended read ordering.

---

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

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

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 0f0c5ad

---

Summary

Well-designed API that reuses the existing ingestion pipeline rather than duplicating it. The approach of adding an optional LsmEditJobSpec to ExternalSstFileIngestionJob minimizes new code while enabling declarative placement. The sequence-order validator and straddler probing are thorough. The test suite covers the main scenarios well.

High-severity findings (1):

• [external_sst_file_ingestion_job.cc: AssignLevelAndSeqnoForIngestedFile] FIFO compaction bypass: the early return for declared_level >= 0 skips the FIFO compaction style check, allowing files to be placed at non-L0 levels in FIFO column families.

Full review (click to expand)

Findings

🔴 HIGH

H1. FIFO compaction style bypass — external_sst_file_ingestion_job.cc

• Issue: The early return added to AssignLevelAndSeqnoForIngestedFile for declared_level >= 0 bypasses the FIFO compaction style check at line ~1666 (compaction_style == kCompactionStyleFIFO). In normal ingestion, FIFO forces all files to L0. With ApplyLsmEdit, a caller could declare files at any level in a FIFO CF, violating FIFO invariants (FIFO only uses L0).
• Root cause: RecordDeclaredLevels validates level bounds and same-level overlap, but does not check compaction_style. The early return in AssignLevelAndSeqnoForIngestedFile skips the compaction-style-aware logic entirely.
• Suggested fix: Add a check in RecordDeclaredLevels (or UseDeclaredLevelForIngestedFile) that rejects non-L0 levels when cfd_->ioptions().compaction_style == kCompactionStyleFIFO. Alternatively, document that ApplyLsmEdit is not supported with FIFO compaction and reject it early. A test with FIFO compaction should be added as well.

🟡 MEDIUM

M1. Missing validation: move_files/link_files mutual exclusion in ApplyLsmEdit — db_impl.cc

• Issue: LsmEditOptions has both move_files and link_files fields. ApplyLsmEdit copies these into IngestExternalFileOptions but does not validate their mutual exclusion before reaching the downstream check. The existing check in PrepareFileIngestionImpl catches this, but the error message will reference "ingest" terminology rather than "LSM edit."
• Suggested fix: Add if (options.move_files && options.link_files) return Status::InvalidArgument(...) at the top of ApplyLsmEdit for a clearer error path.

M2. ValidateDeclaredLevelSequenceOrder is O(n² × file_size) worst case — external_sst_file_ingestion_job.cc

• Issue: Pairwise comparison of all files, opening TableReaders for overlapping pairs. With many overlapping L0 files, this is O(n²) pairs × O(keys) per pair. The code acknowledges this with a comment.
• Suggested fix: The comment is sufficient for an EXPERIMENTAL API. Consider documenting the cost in LsmEdit or DB::ApplyLsmEdit API docs so callers of large edits are aware.

M3. No PERF_TIMER or statistics for straddler probing — external_sst_file_ingestion_job.cc:ProbeStraddlingFiles

• Issue: ProbeStraddlingFiles performs I/O (seeks, range tombstone reads) but has no fine-grained performance instrumentation beyond the outer PERF_TIMER_GUARD(file_ingestion_nanos) in ApplyLsmEdit. A statistics counter for straddling-file probes would aid operational debugging.
• Suggested fix: Consider adding a RecordTick or a PERF_TIMER for the probing step.

M4. Test coverage gap: no FIFO or Level compaction style tests — lsm_edit_test.cc

• Issue: All tests use the default compaction style. No test verifies behavior with FIFO compaction (see H1) or explicitly with Level compaction. Missing tests for move_files = true, concurrent compaction conflict detection, and memtable flush interaction (allow_blocking_flush).
• Suggested fix: Add tests for: (1) FIFO compaction rejection, (2) move_files option, (3) range overlap with pending compaction (Status::InvalidArgument expected), (4) memtable overlap requiring flush.

M5. DeclaredFileFitsInLevel does not check files added by earlier files in the same edit — external_sst_file_ingestion_job.cc

• Issue: DeclaredFileFitsInLevel checks the current VersionStorageInfo minus deleted files, but does not account for files added by the same edit. If two files in the same LsmEdit are declared at the same non-L0 level but don't overlap each other, RecordDeclaredLevels accepts them (correct). But DeclaredFileFitsInLevel only checks against existing files, not against previously-added-in-this-edit files. This is actually safe because same-level-no-overlap is validated in RecordDeclaredLevels, and DeclaredFileFitsInLevel checks surviving existing files. The newly added files from this edit are not yet in VersionStorageInfo. So overlapping is prevented by RecordDeclaredLevels and surviving-file conflicts are prevented by DeclaredFileFitsInLevel. This is correct.
• Suggested fix: None needed - the two checks are complementary.

🟢 LOW / NIT

L1. LsmEditFile::level defaults to 0 — include/rocksdb/lsm_edit.h

• Issue: Default level = 0 means a caller who forgets to set the level gets L0. This is safe but could be surprising. Consider whether -1 (invalid, requiring explicit setting) would be better for catching caller mistakes.
• Suggested fix: Consider documenting the default prominently, or using a sentinel value.

L2. Test dedup opportunity — lsm_edit_test.cc

• Issue: Tests RejectsOlderSequenceNumberInHigherLevel, AllowsInterleavedSequenceRangesForDistinctKeys, RejectsOlderRangeTombstoneInHigherLevel, and RejectsEqualSequenceNumberAcrossLevels all create a source DB, flush files, extract paths, and close. A helper function would reduce ~30 lines per test.
• Suggested fix: Extract a helper like MakeSourceFiles(DB& source, ...) that returns file paths.

L3. Release note and build system updates are complete

• The release note is concise and user-focused. BUCK, CMakeLists.txt, Makefile, and src.mk all updated. All DB subclasses (DBImpl, DBImplReadOnly, DBImplSecondary, StackableDB, ModelDB) have the new virtual method.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES (via DB virtual)	YES — files keep own seqnos, read_callback handles visibility	Safe
ReadOnly DB	Returns NotSupported	N/A	Handled
Secondary DB	Returns NotSupported	N/A	Handled
User-defined timestamps	Rejected in RecordDeclaredLevels	N/A	Handled
FIFO compaction	YES — NOT GUARDED	NO — FIFO expects L0 only	See H1
Universal compaction	YES	YES — atomic_replace_range machinery reused	Safe
Level compaction	YES	YES	Safe
BlobDB (via StackableDB)	Forwarded to inner DB	YES	Safe
Concurrent memtable writes	Handled by flush + write-thread stall	YES	Safe
Old snapshots	Safe — files keep own seqnos, snapshot reads use normal seqno visibility	YES	Safe

Assumption stress-test results:

1. "Edit range holds no live keys": Protected by (a) memtable flush check + write-thread stall, (b) straddler probing, (c) contained-file deletion. Concurrent compaction output is guarded by RangeOverlapWithCompaction check.

2. "Sequence numbers correctly ordered": Validated by LsmEditSequenceOrderValidator which checks point-vs-point, tombstone-vs-point, and tombstone-vs-tombstone. The pairwise check is correct for transitive ordering because: if A > B and B > C at every overlapping key, then A > C holds. The validator checks all overlapping pairs.

3. "L0 ordering is correct": L0 files are ordered by epoch number (cfd_->NewEpochNumber()), which is assigned in AssignLevelsForOneBatch at line ~908. Files in add_files order get ascending epoch numbers because they're processed sequentially and each calls NewEpochNumber(). The "later = newer" claim holds.

Positive Observations

• Clean API design: LsmEditOptions / LsmEdit / LsmEditFile are well-structured and follow existing RocksDB patterns.
• Thorough sequence validation: The LsmEditSequenceOrderValidator is more rigorous than what normal ingestion performs, checking all combinations of point entries and range tombstones.
• Elegant straddler probing: A single seek + range tombstone check avoids the need to compact just to move a file boundary.
• Good code reuse: Building on ExternalSstFileIngestionJob rather than creating a parallel path avoids duplicating complex flush, compaction-conflict, and MANIFEST-commit logic.
• Comprehensive test suite: 21 test cases covering positive paths, error paths, multi-CF atomicity, cross-level sequence ordering, reopening, and read-only/secondary rejection.
• All DB subclasses updated: StackableDB, DBImplReadOnly, DBImplSecondary, and ModelDB all have the new virtual method override.

---

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

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

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit c2b2b9a

---

Summary

Well-designed new API that fills a real gap in the file ingestion surface. The implementation cleanly reuses ExternalSstFileIngestionJob with a mode flag rather than duplicating the ingestion pipeline. The sequence-number ordering validator is thorough, and the straddler probing logic is correct. Test coverage is good.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. ValidateDeclaredLevelSequenceOrder quadratic worst case — db/external_sst_file_ingestion_job.cc

The nested loop in ValidateDeclaredLevelSequenceOrder is O(n²) in the number of overlapping file pairs. When many L0 files overlap with each other AND have ambiguous sequence ranges (requiring table scans), each pair triggers two full table scans. The code comment acknowledges this and suggests a "tagged multiway scan" for the future.

• Issue: For a large number of overlapping L0 files with identical seqno ranges (e.g., all seqno=0 from SstFileWriter), this is quadratic in file count. For typical use cases (transplanting a scratch DB) this is likely manageable, but the API documentation should mention this cost.
• Suggested fix: The LsmEdit::add_files documentation already partially covers this ("producing quadratic work in the worst case"). Consider adding a cap or at least logging a warning when the number of pairs exceeds a threshold.

M2. Missing db_stress coverage

Per CLAUDE.md guidelines: "When adding a new feature, make sure stress test covers the new option." The PR does not add any db_stress integration for ApplyLsmEdit. While the feature is EXPERIMENTAL, stress testing would catch concurrency issues with background compactions, flushes, and other ingestions happening concurrently.

• Suggested fix: Add ApplyLsmEdit calls to db_stress in a future PR, at minimum exercising the basic graft-into-empty-range pattern.

M3. No db_bench support

Per CLAUDE.md: "When adding a performance related feature, support it in db_bench." ApplyLsmEdit is performance-related (its raison d'être is avoiding unnecessary I/O). No db_bench benchmark is added.

• Suggested fix: Add a db_bench benchmark for ApplyLsmEdit in a follow-up, particularly to measure the straddler probing cost.

M4. BlobDB interaction not guarded — db/external_sst_file_ingestion_job.cc

If a source DB uses BlobDB, its SST files may contain kTypeBlobIndex entries referencing blob files. ApplyLsmEdit would install the SST files but the referenced blob files would not be present, leading to read errors when those keys are accessed. There is no check for this in the current code.

• Suggested fix: Either reject files with blob references (check table_properties.num_blob_references > 0 or similar), or document this as a known limitation of the EXPERIMENTAL API.

🟢 LOW / NIT

L1. ProbeStraddlingFiles does not log which straddling file was probed

When a straddling file is successfully probed (no keys in range), there is no log message. Only failures produce a user-visible error. Adding an INFO-level log for successful probes would help debugging production issues.

L2. Universal compaction not explicitly tested

The SupportsAtomicReplaceRangeTombstone function requires Universal compaction for the range-tombstone fallback. Since ApplyLsmEdit never uses that path (it uses ProbeStraddlingFiles instead), Universal compaction + ApplyLsmEdit works correctly but has no dedicated test. Consider adding one to prevent regressions.

L3. ReplaceWholeColumnFamily test omits range specification

In ReplaceWholeColumnFamily, the LsmEdit sets no range (both endpoints unset), meaning "whole column family." This is the only test exercising the unbounded range path for ApplyLsmEdit. Consider adding a test that verifies the behavior when the whole CF is replaced and there are files at multiple levels.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES (via StackableDB)	YES - no read_callback interaction	Safe
ReadOnly DB	Returns NotSupported	N/A	None
SecondaryInstance	Returns NotSupported	N/A	None
User-defined timestamps	Rejected in RecordDeclaredLevels	N/A	None
BlobDB	SST files installed, blob files missing	Potentially unsafe	See M4
FIFO compaction	Correctly requires level 0	YES	None
Universal compaction	Works via probe path	YES	Add test (L2)
Snapshots	Range verified empty; seqnos preserved	YES	Safe
Concurrent writers	Flush check + mutex in Run()	YES	Safe

Positive Observations

1. Clean reuse of existing infrastructure. The PR adds a small mode flag (LsmEditJobSpec) to ExternalSstFileIngestionJob rather than building a parallel pipeline. All existing safeguards (memtable overlap, compaction conflict, file copy/link, checksum, MANIFEST write) are inherited.

2. Thorough sequence-number validation. LsmEditSequenceOrderValidator checks all four combinations (point-vs-point, tombstone-vs-point, point-vs-tombstone, tombstone-vs-tombstone) with a merge-like scan. The fast path skipping table scans when seqno ranges are disjoint is a good optimization.

3. Good API design. The LsmEdit/LsmEditFile/LsmEditOptions structs are well-documented and consistent with existing RocksDB patterns. The separation between per-call options and per-edit parameters is clean.

4. Straddler probing is elegant. One seek per file + range-tombstone check avoids rewriting data that is already correct.

5. Comprehensive test coverage. 18 test cases covering: basic grafting, rejection of non-empty ranges, range tombstones, straddler probe disable, level occupancy, removed-file reuse, whole-CF replace, cross-CF atomicity, rollback on failure, L0 ordering, overlapping files, sequence number validation (3 variants), bad arguments, FIFO, incomplete checksums, full LSM transplant, reopen durability, and read-only/secondary rejection.

6. Build system updates are complete. Makefile, CMakeLists.txt, BUCK, and src.mk all updated.

---

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

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

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit baa2565

---

Summary

This is a large and complex PR (~4500 insertions across 28 files) adding DB::ApplyLsmEdit(), a declarative LSM edit primitive. The PR also makes two changes that affect ALL workloads globally: modifying the merging iterator comparator and removing the is_key_seqnum_zero_ optimization in DBIter. The LSM edit functionality builds on the existing file ingestion infrastructure with extensive validation logic.

High-severity findings (3):

• [table/merging_iterator.cc:537, table/compaction_merging_iterator.cc:200] Global merging iterator comparator change drops value-type tiebreaking, affecting ALL reads/compactions. When two entries share user_key + seqno but differ in type (e.g., SingleDeletion vs Put), the old deterministic type-based ordering is replaced with level-based ordering, which may change observable merge semantics.
• [db/db_impl/db_impl.cc:8141] bg_cv_.SignalAll() condition uses || (num_running_ingest_file_ == 0 || num_running_lsm_edits_ == 0), causing spurious wakeups in GetSnapshotImpl whenever any plain ingestion finishes, even if LSM edits are still running.
• [db/db_impl/db_impl.cc:7701-7730] PrepareFileIngestionImpl holds the DB mutex while creating ingestion jobs and initializing snapshot contexts for all column families. With many CFs or merge-existing-files enabled, this extends the critical section with allocation and snapshot-list copying.

Full review (click to expand)

Findings

🔴 HIGH

H1. Global merging iterator comparator change drops value-type tiebreaking -- table/merging_iterator.cc:537

• Issue: The MinHeapItemComparator and MaxHeapItemComparator in MergingIterator (and CompactionMergingIterator) changed from Compare() (which includes value type in tiebreaking) to CompareKeySeq() (which ignores value type) plus level-based tiebreaking. This affects every read, scan, compaction, and flush in RocksDB, not just LSM edits.

  Compare() orders by: user_key, then decreasing (seqno << 8 | type). So for same user_key + same seqno, it ordered by decreasing type value (e.g., kTypeSingleDeletion=7 before kTypeValue=1).

  CompareKeySeq() orders by: user_key, then decreasing seqno only (shifts out the type byte). For same user_key + same seqno, it returns 0, and the fallback a->level > b->level orders by child index.

• Root cause: The change was made to ensure that for equal-seqno entries (as produced by ApplyLsmEdit), the entry from the higher (newer) level wins, matching the declared priority. However, same user_key + same seqno entries with different value types can occur in normal operation (e.g., SingleDeletion + Put written in the same batch, or after sequence normalization). The old type-based ordering was relied upon to present SingleDeletion before the Put it covers.

• Suggested fix: Either (a) restore type-based tiebreaking when levels are equal (i.e., return cmp != 0 ? cmp > 0 : (a->level != b->level ? a->level > b->level : comparator_->Compare(...) > 0)), or (b) provide strong evidence that no consumer relies on type ordering for same-seqno entries and document the behavioral change.

H2. Spurious bg_cv_.SignalAll() on num_running_ingest_file_ reaching zero -- db/db_impl/db_impl.cc:8141

• Issue: The condition:

  if (0 == num_running_ingest_file_ || 0 == num_running_lsm_edits_) {
      bg_cv_.SignalAll();
  }

  signals all waiters whenever either counter reaches zero. GetSnapshotImpl (line 5583) waits on num_running_lsm_edits_ > 0 using bg_cv_.Wait(). If a plain ingestion finishes first (setting num_running_ingest_file_ to 0), this signals the condvar, waking GetSnapshotImpl. It rechecks the while condition and goes back to sleep, but this is a spurious wakeup that wastes CPU under contention.

  More importantly, the old code was if (0 == num_running_ingest_file_). The || means the signal now fires whenever either counter hits zero, which is more often than before. This could cause thundering herd effects when many snapshots are waiting.

• Root cause: The two counters were conflated into one signal condition. They should be independent: bg_cv_.SignalAll() should fire when the counter that waiters actually check reaches zero.

• Suggested fix: Use && instead of ||, or signal only when the specific counter that changed reaches zero:

  if (lsm_edits_started && num_running_lsm_edits_ == 0) {
      bg_cv_.SignalAll();
  }
  if (num_running_ingest_file_ == 0) {
      bg_cv_.SignalAll();
  }

H3. DB mutex held during ingestion job construction with potential I/O -- db/db_impl/db_impl.cc:7701-7730

• Issue: PrepareFileIngestionImpl acquires the DB mutex to get SuperVersions and create ingestion jobs:

  InstrumentedMutexLock options_lock(&mutex_);
  // For each CF: InitSnapshotContext (copies snapshot list), GetSuperVersion, Ref, create job

  InitSnapshotContext allocates a JobContext, copies the snapshot list, and may take a managed snapshot. The ingestion job constructor copies MutableDBOptions and EnvOptions. With many column families or when merge_existing_files is enabled, this critical section includes non-trivial allocation and copying.

• Root cause: The snapshot context must be initialized under mutex for consistency, but the job objects themselves could be created outside the lock.

• Suggested fix: Minimize the locked region: acquire SuperVersion refs and snapshot context under mutex, then create the job objects after releasing.

🟡 MEDIUM

M1. MarkFilesBeingCompacted(true) on deleted files may assert if file is already compacting -- db/external_sst_file_ingestion_job.cc:2310

• Issue: CreateEquivalentFileIngestingCompactions adds deleted live files to synthetic Compaction objects:

  for (const auto& [level, file] : deleted_file_compaction_inputs_) {
      input.files.push_back(const_cast<FileMetaData*>(file));
  }

  The Compaction constructor calls MarkFilesBeingCompacted(true), which asserts being_compacted != inputs_[i][j]->being_compacted. If a file is already being compacted (despite the range overlap check passing), this will fire in debug builds.

  The range overlap check (RangeOverlapWithCompaction) happens before CreateEquivalentFileIngestingCompactions, but there's no per-file being_compacted check. A compaction on a disjoint range at the same level wouldn't be caught by the range check but could have files that overlap the deleted set due to different range definitions.

• Suggested fix: Add an explicit being_compacted check in DeleteLiveFile or AddExplicitFileDeletions, returning Status::TryAgain if the file is already being compacted.

M2. const_cast<FileMetaData*> of deleted file pointers risks UB -- db/external_sst_file_ingestion_job.cc:2310

• Issue: deleted_file_compaction_inputs_ stores const FileMetaData* from the Version's storage info. The const_cast to non-const is used because CompactionInputFiles::files expects FileMetaData*. While MarkFilesBeingCompacted only modifies being_compacted (a mutable bool), the const_cast itself is a code smell that could mask bugs if the Compaction ever modifies other fields.

• Suggested fix: Consider making being_compacted mutable and using const FileMetaData* in the Compaction inputs for this case, or adding a comment documenting why the cast is safe.

M3. LsmEditMergeIterator dedup may incorrectly skip SingleDeletion entries -- db/external_sst_file_ingestion_job.cc:477-487

• Issue: The merge iterator's Next() skips entries with the same user_key and sequence as the winner when the winner is not a merge operand:

  while (!merge_operand && Valid() &&
         ucmp_->Compare(previous_user_key_, ...) == 0 &&
         seq == inputs_[heap_.front()].parsed.sequence) {
      Advance();
  }

  If the winner is a Put and a SingleDeletion with the same seqno exists in a different input, the SingleDeletion is skipped. This could leave data that should have been single-deleted.

  However, since zero_sequence_numbers rejects merge operands but not SingleDeletions (it converts them to regular Deletions in Refresh), and equal-seqno dedup is specifically for handling cross-DB snapshot imports, this may be acceptable. The PR description says "later inputs resolve equal-sequence point overwrites."

• Suggested fix: Document that SingleDeletions are treated as plain Deletions during merge (as Refresh already does for zero_sequence_numbers) or verify that the dedup behavior is correct for all type combinations.

M4. Double BlockNew() / UnblockNew() nesting correctness -- db/db_impl/db_impl.cc:7978-8139

• Issue: For LSM edit jobs, BlockNew() is called twice (once for LSM edits, once for regular ingestion) and UnblockNew() is called twice. The ordering is:

  1. BlockNew for LSM edits (line 7980)
  2. BlockNew for regular ingestion (line 7985)
  3. UnblockNew for regular ingestion (line 8108)
  4. UnblockNew for LSM edits (line 8139)

  Between steps 3 and 4, new snapshots remain blocked because the LSM edit BlockNew is still active. Between these lines, num_running_lsm_edits_ is still > 0. GetSnapshotImpl checks both the BlockNew gate AND num_running_lsm_edits_ under mutex. This seems correct but fragile.

  The real concern: if status is not ok, snapshots_blocked may be false (line 7983) but lsm_edits_started is true (line 7978). Then BlockNew is called once (for LSM edits) but UnblockNew is also called once (for LSM edits at line 8139). However, snapshots_blocked uses status.ok() AFTER lsm_edits_started was set. If Run() fails, both flags track independently and unblock independently. This appears correct but is hard to reason about.

• Suggested fix: Add a comment explaining the double-blocking design and invariants.

M5. SelectExistingMergeInputs doesn't check being_compacted -- db/external_sst_file_ingestion_job.cc:1790-1820

• Issue: Files selected for merge_existing_files are checked for being_compacted later in AddMergedExistingFileDeletions, but by then the merge output has already been built (potentially expensive I/O). A file becoming being_compacted between selection and the check wastes the merge work.

• Suggested fix: Check being_compacted during selection to fail fast, or document that the late check is intentional because the merge output can be discarded cheaply.

M6. CLAUDE.md violation: defaulted parameter in constructor -- db/external_sst_file_ingestion_job.h:262

• Issue: The ExternalSstFileIngestionJob constructor has a new defaulted parameter:

  std::shared_ptr<JobContext> merge_snapshot_context = nullptr

  CLAUDE.md explicitly states: "Avoid new defaulted parameters. This is the Miss Spelling in README #1 trap on refactoring!"

• Suggested fix: Use a separate overload or require all callers to pass the parameter explicitly.

M7. Performance regression from is_key_seqnum_zero_ removal -- db/db_iter.cc:551

• Issue: The is_key_seqnum_zero_ optimization skipped CompareKeyForSkip() (a user key comparison) when the previous entry had sequence 0. On the bottommost level where virtually all entries have seqno 0, this saved one comparison per entry during forward iteration. This optimization is now unconditionally removed, affecting all workloads.

  The optimization was sound for the invariant that seqno-0 means one entry per user key (after full compaction). The PR removes it because ApplyLsmEdit can install multiple seqno-0 entries for the same key. However, this only happens with the new feature - the optimization could be preserved with a flag.

• Suggested fix: Consider a per-SuperVersion or per-read-path flag that disables the optimization only when seqno-0 LSM edits have been installed, rather than removing it universally.

🟢 LOW / NIT

L1. Missing release note for global behavioral changes -- unreleased_history/new_features/apply_lsm_edit.md

• Issue: The release note covers ApplyLsmEdit but does not mention the merging iterator comparator change or the is_key_seqnum_zero_ removal, both of which affect all workloads. These are behavioral changes per CLAUDE.md's Behavioral Compatibility requirements.

• Suggested fix: Add a separate unreleased_history/behavior_changes/ entry for each global change, even if minimal.

L2. ReadOptions not initialized with io_activity in validators -- db/external_sst_file_ingestion_job.cc:52

• Issue: LsmEditSequenceOrderValidator::read_options_ does not set io_activity. The PR's BuildMergedFile sets io_activity = Env::IOActivity::kCompaction for its ReadOptions. Validators should use a consistent IO activity hint.

L3. autovector default-constructed iterator safety -- util/autovector.h:63

• Issue: Adding iterator_impl() = default; leaves vect_ uninitialized (likely null) and index_ uninitialized. This is fine for the std::stable_sort use case (libc++ may default-construct temporary iterators), and the test covers both inline and heap storage paths. The default-constructed iterator must never be dereferenced, only assigned to.

L4. LsmEditMergeIterator heap uses std::vector instead of custom binary heap -- db/external_sst_file_ingestion_job.cc:458

• Issue: The LsmEditMergeIterator uses std::make_heap/std::push_heap/std::pop_heap on a std::vector, while the production MergingIterator uses a custom cache-line-aligned binary heap. For merge operations, this is acceptable since it runs during preparation (not hot path), but the string allocations in Refresh() could be optimized.

Cross-Component Analysis

Context	Affected?	Assessment
WritePreparedTxnDB	YES - snapshot creation waits	The bg_cv_.Wait() in GetSnapshotImpl adds latency when LSM edits are running. WritePreparedTxnDB creates snapshots on its read path. If an LSM edit is running concurrently, reads may stall. The PR description mentions "avoiding the WritePrepared internal-snapshot deadlock."
ReadOnly DB	NO	Returns NotSupported
Secondary Instance	NO	Returns NotSupported
FIFO compaction	Partially - L0 only	Non-L0 placement rejected; L0 edits allowed
BlobDB	Partially	External blob references rejected; inline/same-file blobs supported via SameFileBlobReader
User-defined timestamps	NO	Rejected by RecordDeclaredLevels
CompactedDB	NO	Returns NotSupported
Concurrent compactions	YES	RangeOverlapWithCompaction check + RegisterCompaction blocking. But per-file being_compacted check for deleted files is only done late (M1).
Concurrent snapshots	YES	Double-blocked during commit. Wait loop in GetSnapshotImpl under mutex.

Positive Observations

1. Thorough validation: The sequence ordering validation (LsmEditSequenceOrderValidator) is comprehensive, checking point entries, tombstones-vs-points, and tombstone-vs-tombstone orderings.

2. Lifecycle safety: Changing mutable_db_options_ and env_options_ from references to values prevents dangling reference bugs when the job outlives the caller's scope.

3. SstFileManager integration: Generated files (merged outputs, tombstones) are properly registered and unregistered with SstFileManager, including space limit checks.

4. Preparation SuperVersion pinning: SuperVersions are pinned during preparation with shared_ptr + custom deleter, preventing use-after-free if a flush replaces the SuperVersion during preparation.

5. Comprehensive error handling: The code validates inputs extensively before doing expensive I/O, and rolls back cleanly on failure.

6. Test coverage: The 2191-line test file exercises a wide range of scenarios including sequence ordering, snapshot consistency, merge modes, straddler probing, and error cases.

---

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

---

Summary

Large, well-structured PR adding an experimental declarative LSM edit API. The core LSM edit logic is generally sound, but the PR makes several changes to shared infrastructure (MergingIterator, CompactionMergingIterator, DBIter) that affect all code paths, not just LSM edits. Two of these changes carry behavioral risk for existing workloads.

High-severity findings (2):

• [table/merging_iterator.cc:538] MergingIterator comparator change from Compare to CompareKeySeq + level tiebreaking affects ALL reads, not just LSM edits; changes tiebreaking semantics for equal user_key+seqno entries with different value types.
• [table/compaction_merging_iterator.cc:200] CompactionMergingIterator removes the assert(a->type == b->type || r != 0) invariant and changes compaction key ordering for equal-seqno entries, affecting all compactions.

Full review (click to expand)

Findings

🔴 HIGH

H1. MergingIterator comparator change affects ALL read paths -- table/merging_iterator.cc:538

• Issue: The MinHeapItemComparator and MaxHeapItemComparator now use CompareKeySeq (user_key + seqno, ignoring value type) plus level-based tiebreaking for ITERATOR-vs-ITERATOR comparisons. This replaces the previous Compare which included value type in the ordering. This change affects every point read, range scan, and Get() operation in RocksDB, not just LSM edit paths.
• Root cause: When two entries have the same user key and sequence number but different value types (e.g., kTypeDeletion vs kTypeValue, or kTypeMerge vs kTypeSingleDeletion), the old code used value type to break the tie deterministically. The new code uses level (input index), which means the ordering depends on which SST the entry came from. For normal operations where equal (user_key, seqno) across different inputs should not happen, the behavioral change may be benign -- but the value type tiebreaker had a defined ordering (higher type number = newer/earlier in iteration), while level-based tiebreaking has a completely different semantic meaning.
• Impact scope: This affects MergingIterator which is used in DBImpl::NewIterator, DBImpl::Get, range scans, and all read-side merging. It is not gated behind any LSM edit flag.
• Suggested fix: The merging iterator change should be conditional on whether the iterator is serving an LSM edit path, or the change should be separated into its own PR with dedicated testing and benchmarking of existing read workloads. At minimum, verify with stress tests that no existing workload depends on the value-type ordering.

H2. CompactionMergingIterator removes safety assertion -- table/compaction_merging_iterator.cc:200

• Issue: The old CompactionHeapItemComparator had assert(a->type == b->type || r != 0) which validated that different-type HeapItems (ITERATOR vs DELETE_RANGE_START) never had equal keys during compaction. This was an important debug invariant. The new code removes this assertion and replaces Compare with CompareKeySeq + explicit type/level tiebreaking, changing behavior for ALL compactions.
• Root cause: LSM edits can produce equal-key/seqno entries across files. However, for non-LSM-edit compactions, the old invariant should still hold (range tombstone start keys should differ from point keys at the same sequence number due to the value type component).
• Impact: The CompareKeySeq change means range tombstone starts now sort before equal-key/seqno points (a->type == HeapItem::ITERATOR returns true, meaning ITERATOR items are "later", so DELETE_RANGE_START items come first). This changes the relative ordering of range tombstone boundaries and point keys during compaction when they have the same user key and sequence number.
• Suggested fix: Keep the assertion for non-LSM-edit compactions. Consider a flag on the compaction or iterator to switch between the old and new comparators.

🟡 MEDIUM

M1. DBIter is_key_seqnum_zero_ removal may cause performance regression -- db/db_iter.cc:551

• Issue: The is_key_seqnum_zero_ optimization allowed DBIter to skip the CompareKeyForSkip call when the previous key had sequence number 0, since a seqno-0 key is the lowest version and the next key is guaranteed to be a different user key in the pre-LSM-edit world. This optimization is removed entirely.
• Root cause: With LSM edits that preserve sequence numbers, multiple files can have seqno-0 entries for the same user key at different levels, breaking the assumption. The fix is correct for correctness.
• Impact: Every forward iteration step now does an extra CompareKeyForSkip call that was previously elided for seqno-0 entries. For bottommost-level scans where all keys have seqno 0, this could be measurable. The optimization was explicitly coded with a detailed comment explaining its value.
• Suggested fix: Benchmark the impact. If measurable, the optimization could be preserved with an additional check (e.g., only skip when no LSM edit has ever been applied to the DB, or when the current SuperVersion's LSM state is known to have standard seqno ordering).

M2. CompactionPicker::RegisterCompaction overload -- db/compaction/compaction_picker.cc:1228

• Issue: The new RegisterCompaction(Compaction* c, bool has_live_l0_inputs) overload is called from the old RegisterCompaction(Compaction* c) with a hardcoded false. Per CLAUDE.md, new defaulted parameters are the "Miss Spelling in README #1 trap on refactoring." While this uses overloading rather than a default parameter, the semantic effect is similar: callers of the old signature silently get false.
• Suggested fix: Document the delegation pattern clearly, or update all callers to use the two-argument form.

M3. mutable_db_options_ and env_options_ changed from references to copies -- db/external_sst_file_ingestion_job.h:582-583

• Issue: const MutableDBOptions& mutable_db_options_ changed to const MutableDBOptions mutable_db_options_ (copy by value). Same for env_options_. This captures a snapshot of options at job construction time.
• Assessment: This is actually correct -- the LSM edit's BuildMergedFile runs during preparation (outside mutex), so references to mutable state would be unsafe. The existing non-LSM-edit ingestion path also reads these options only during preparation and commit, so the snapshot semantics are equivalent. No behavioral change for existing callers since the options don't change between job creation and use.
• Suggested fix: Add a comment explaining the copy is intentional for lifetime safety.

M4. Flush wait loop with no clear bound -- db/db_impl/db_impl.cc:8258

• Issue: while (num_running_flushes_ > 0) { bg_cv_.Wait(); } waits for all running flushes to complete when LSM edits are active with snapshot checker or custom GC. Since the DB mutex is held, new flushes should not be scheduled, but any in-progress flush background thread must complete before proceeding.
• Assessment: Safe because: (1) mutex is held so no new flush jobs are scheduled, (2) running flushes will eventually complete and signal bg_cv_. But the comment should clarify that this drains existing flushes, not all future ones.

M5. ScopedDeferredFileIOListeners uses O(n*m) listener matching -- file/file_util.cc:97-108

• Issue: The constructor and Notify paths do linear scans through scopes and listeners for matching. This is acceptable for the expected small number of listeners but should be documented.

M6. LiveFileInfo disables sequence order scan fast path -- db/external_sst_file_ingestion_job.cc:587

• Issue: LiveFileInfo sets entry_stats_initialized = false, which forces CanSkipSequenceOrderScan to return false, requiring a full table scan to validate sequence ordering between added and surviving files. The entry count IS available via FileMetaData and TableProperties, so the fast path could be enabled.
• Suggested fix: Populate entry stats from available metadata to enable the fast path more often, reducing I/O during validation.

🟢 LOW / NIT

L1. Release note is too detailed -- unreleased_history/new_features/apply_lsm_edit.md

• Issue: Per CLAUDE.md, release notes should be "kept short at high level for external user consumption." The current note is 9 paragraphs with implementation detail. Prior agents have been warned: "PLEASE learn from past agents who tried to build elaborate release notes."
• Suggested fix: Reduce to: "Added experimental DB::ApplyLsmEdit() for atomically adding and deleting SST files at caller-declared levels across column families. Optional merging compacts incoming files per level. See include/rocksdb/lsm_edit.h."

L2. bg_cv_ signal uses || -- db/db_impl/db_impl.cc:8435

• Issue: if (0 == num_running_ingest_file_ || 0 == num_running_lsm_edits_) signals when either counter hits zero, causing some spurious wakeups.
• Assessment: This is consistent with bg_cv_ usage throughout the codebase -- it's a shared condition variable where all waiters re-check their specific conditions in while loops. The || is correct: each counter has its own set of waiters, and signaling when either reaches zero ensures the right waiter is woken.

L3. autovector::iterator_impl default constructor -- util/autovector.h:63

• Issue: iterator_impl() = default; is added to support std::stable_sort, which requires default-constructible iterators. The test validating this is well-written.

L4. const_cast in CreateEquivalentFileIngestingCompactions -- db/external_sst_file_ingestion_job.cc:2343

• Issue: input.files.push_back(const_cast<FileMetaData*>(file)) casts away const from deleted_file_compaction_inputs_ entries. The Compaction constructor takes non-const FileMetaData* for its inputs, so the cast is mechanically required, but it weakens the const-correctness of the deleted file references.

Cross-Component Analysis

Context	Affected?	Assessment
WritePreparedTxnDB	No (LSM edit does not use read_callback)	Safe
ReadOnly DB	Explicitly rejected	Safe
SecondaryInstance	Explicitly rejected	Safe
CompactedDB	Explicitly rejected	Safe
User-defined timestamps	Explicitly rejected in RecordDeclaredLevels	Safe
FIFO compaction	Only L0 permitted	Safe
MergingIterator change	ALL reads and compactions	Risk (H1, H2)
DBIter change	ALL forward iteration	Performance risk (M1)
Snapshot blocking	All snapshot creation	Safe (properly paired BlockNew/UnblockNew)
StackableDB	Passthrough override added	Safe

Positive Observations

• The API design is clean and well-documented, with clear separation between LsmEditOptions (per-call) and LsmEdit (per-CF).
• Extensive input validation: sequence order between overlapping files, level-fit checking, straddling file probing, external blob reference rejection, checksum consistency.
• Proper rejection for unsupported configurations (read-only, secondary, timestamps, compacted DB).
• The ScopedDeferredFileIOListeners mechanism for preventing I/O callback re-entrancy and deadlock is a thoughtful solution. The thread-local callback stack and IsInCallback check are well-designed.
• Build system properly updated across all four systems (Makefile, CMakeLists.txt, src.mk, BUCK).
• The entry_stats_initialized flag correctly distinguishes "zero entries" from "entry count unknown."
• The snapshot blocking with num_running_lsm_edits_ counter is correctly paired (two BlockNew + two UnblockNew when LSM edits are active, matching the ShardedSnapshotList counter semantics).
• The deletion reservation cleanup fix (MarkFilesBeingCompacted(false) before UnregisterCompaction, plus ComputeCompactionScore refresh) correctly addresses the picker bookkeeping issue.
• The LsmEditMergeIterator properly handles blob resolution via GetSameFileBlobReader, zero_sequence_numbers reset, and equal-sequence point deduplication.

---

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