[xingbowang]

Summary:
Why: CreateColumnFamilyWithImport only brings SST files, never blob files.
If the imported SSTs hold blob references, the new column family points to
blob files that do not exist there. Reads of those keys fail later, and the
problem is not visible at import time.

Previous behavior: import accepted both of these cases:

• The new column family has enable_blob_files = true.
• Imported files reference blob files, for example an export
  (Checkpoint::ExportColumnFamily) of a BlobDB column family. Import also
  wrote kInvalidBlobFileNumber as the new file's oldest blob file number,
  which hid the link while the blob indexes stayed in the SST.

New behavior: CreateColumnFamilyWithImport returns NotSupported in both
cases, before it creates the column family. So a rejected import leaves no
column family behind. A normal import does not change.

Implementation:

• DBImpl::CreateColumnFamilyWithImport checks options.enable_blob_files,
  and checks each imported file's oldest_blob_file_number in the metadata
  loop that already exists. The checks read only metadata (no file reads),
  cost O(number of files), and run before any DB mutex is taken.
• Doc line on DB::CreateColumnFamilyWithImport in include/rocksdb/db.h.
• Release note under unreleased_history/behavior_changes.

Limits and unsupported cases:

• The check trusts the metadata. Hand-built metadata that leaves
  oldest_blob_file_number at 0 for an SST that holds blob indexes is not
  detected, because the check does not read data. SstFileWriter cannot write
  blob indexes, so its files are not affected.
• Checkpoint::ExportColumnFamily is not changed. It still exports a column
  family whose SSTs reference blob files, but it copies only the SSTs. After
  this change such an export can never be imported.
• enable_blob_files can still be turned on with SetOptions after import.
  New blob files then belong to the new column family, so no reference
  dangles.

Changes visible to existing users: BlobDB users who import into a blob
column family, or who import an export of a BlobDB column family, now get
NotSupported. Before, they got a column family with dangling blob references.

Differential Revision: D131129385

[meta-codesync]

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

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 330.2s.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 8998980

---

Summary

Clean, well-scoped defensive change that correctly rejects CreateColumnFamilyWithImport when blob files would create dangling references. The checks are placed before column family creation (no orphan CFs on rejection), before the DB mutex (no performance impact), and cover all call paths through the vector overload. Test coverage is solid. No high-severity issues found.

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Doc comment placement covers only the single-metadata overload -- include/rocksdb/db.h:2778-2780

• Issue: The new doc lines are added to the comment block of the single-metadata overload (the convenience wrapper). The vector overload at line 2796 (the pure-virtual one that subclasses override, and where the actual implementation lives) has no mention of the blob-file restriction. A developer reading only the vector overload's doc would not know about this constraint.
• Suggested fix: Add a brief note to the vector overload's comment block as well, or move the blob-file note to the vector overload (since the single-metadata overload just delegates).

M2. enable_blob_files check may be overly broad for SstFileWriter-produced files -- db/db_impl/db_impl.cc:8779

• Issue: The enable_blob_files option check rejects import even when the imported files themselves contain no blob references. The PR description states this is intentional ("the new column family must not use blob files, even if the imported files do not reference any"), because future writes to the CF would create blob files. However, users can achieve the same result by calling SetOptions after import to enable blob files. This asymmetry could be confusing. The PR description acknowledges this.
• Root cause: Design choice, not a bug. The check is a "fail loud" guard.
• Suggested fix: Consider whether the comment in the code should mention that SetOptions can enable blob files post-import, to prevent confusion from future maintainers.

🟢 LOW / NIT

L1. Test verifies error message substring but not the enable_blob_files error message -- db/import_column_family_test.cc

• Issue: The test checks s.getState() for "reference blob files" on the second rejection (metadata with blob refs), but does not check the error message text for the first rejection (enable_blob_files). This is minor since IsNotSupported() is sufficient.
• Suggested fix: Optional: add a similar getState() check for the first rejection path for consistency.

L2. Pre-existing C-style cast on the same loop -- db/db_impl/db_impl.cc:8783

• Issue: Not introduced by this PR, but the new blob-file check lives in the same loop as (LiveFileMetaData*)&file, a C-style cast. CLAUDE.md prefers static_cast_with_check or similar. This is not a problem with this PR.
• Suggested fix: Out of scope for this PR; note for future cleanup.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
WritePreparedTxnDB	Yes (via PessimisticTransactionDB)	Yes - delegates to DBImpl	Safe
ReadOnly DB	No - returns NotSupported independently	N/A	Safe
CompactedDB	No - returns NotSupported independently	N/A	Safe
StackableDB	Yes - delegates to underlying DB	Yes	Safe
C API	Yes - calls single-metadata overload	Yes - covered	Safe
Java JNI	Yes - calls single-metadata overload	Yes - covered	Safe

DB Mutex Analysis: The new checks execute at lines ~8776-8794, before the first mutex acquisition at line 8809. No DB mutex is held during the new validation. No impact on critical section duration.

Behavioral Compatibility:

• Old behavior: Import silently succeeded, creating a CF with dangling blob references that would cause read failures later.
• New behavior: Import returns NotSupported early, before creating the CF. No orphan CF is left behind.
• Impact assessment: Users who were importing BlobDB exports would see a new error. However, the old behavior was already broken (reads would fail), so this is a strict improvement. The release note accurately describes the change.

Positive Observations

1. Correct ordering: Both checks are placed before CreateColumnFamily(), so a rejected import leaves no orphan column family behind. This avoids the cleanup path (DropColumnFamily/DestroyColumnFamilyHandle) that the function uses for later failures.

2. Consistent with existing pattern: The import job already wrote kInvalidBlobFileNumber at line 253 of import_column_family_job.cc, silently discarding blob references. The new checks make this rejection explicit and early.

3. Test quality: The test covers both rejection paths (enable_blob_files option and metadata with blob refs), verifies the successful import still works after a rejected one, verifies error message content, and checks that rejected imports leave no CF behind. The test reuses existing fixture members (export_files_dir2_, metadata_ptr2_, import_cfh2_) that are properly cleaned up by the destructor.

4. Release note: Concise and accurate, describes both old and new behavior clearly.

5. Scope discipline: The PR correctly limits itself to the import path and does not attempt to change ExportColumnFamily or other related APIs.

---

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