[pdillinger]

Summary:
X-link: #15472

Why: With max_open_files = -1 and open_files_async = true, calling GetPropertiesOfAllTables() (or similar) soon after DB::Open reads each not-yet-opened SST's footer, metaindex and properties block, and then the background opener reads that metadata again when it opens the file. Concurrent callers each repeat the read too, because this path does not coalesce. In this mode every live SST gets opened anyway, so the separate read is pure overhead.

Previous behavior: Version::GetTableProperties() called TableCache::GetTableProperties() with no_io = true and, on a miss, opened a fresh file reader to read only the properties block (counted in NUMBER_DIRECT_LOAD_TABLE_PROPERTIES). Everything that asks a Version for table properties goes through it: GetPropertiesOfAllTables(), GetPropertiesOfTablesInRange(), GetPropertiesOfTablesByLevel(), the aggregated-table-properties DB properties, Compaction::InitInputTableProperties() (compaction and CompactionJobInfo), the seqno-to-time collection in CompactionJob, and MaybeInitializeFileMetaData(). With synchronous open and max_open_files = -1 the lookup always hits, because readers are pinned during recovery and LogAndApply. So in practice the miss happens with open_files_async, before the background opener reaches a file (primary, read-only, or async secondary).

New behavior: When the VersionSet opens all table files (max_open_files = -1), Version::GetTableProperties() calls TableCache::GetTableProperties() with no_io = false, which opens a not-yet-open file into the table cache. The open uses the background opener's parameters (the file's level, read histogram and temperature, MaxFileSizeForL0MetaPin(), no index/filter prefetch), because with -1 that reader stays in the cache for the life of the DB. FindTable()'s per-file loader mutex coalesces this with a concurrent open by the background opener or a reader. The background opener then finds the reader in the cache and only pins it, so each file's metadata is read once.

With max_open_files > 0, nothing changes: a cached reader is used if present, otherwise the properties block is read directly, so these calls do not use up table cache capacity.

Implementation:

• TableCache::GetTableProperties() takes the open parameters that vary by caller: level, file_read_hist and prefetch_index_and_filter_in_cache, all non-defaulted. no_io loses its default, so every caller states all four. It also passes the file's own temperature and MaxFileSizeForL0MetaPin(mutable_cf_options) to FindTable(), as the read paths do.
• Version::GetTableProperties() uses it with no_io = !OpensAllTableFiles() and no index/filter prefetch. It falls back to the direct properties read only on an Incomplete cache miss, as before.
• New VersionSet::OpensAllTableFiles(): true when the table cache is unbounded. ReactiveVersionSet returns false with trusted-MANIFEST recovery, i.e. the DB::OpenAndCompact worker (the default with remote_compaction_manifest_floor). That worker runs with an unbounded table cache but opens only the files its compaction reads, so opening a file just for its properties would not save a later open. It would also move the open's reads out of the remote job's measured bytes_read, which CompactionServiceTest.BasicCompactions checks.
• Version::GetTableProperties() and Version::MaybeInitializeFileMetaData() take the file's level as a new non-defaulted parameter. Every caller already knows it.
• The other two TableCache::GetTableProperties() callers keep index/filter prefetch on, as before. DBImpl::GetPreparedFileInfoForExternalSstIngestion() now passes its live file's level and read histogram. The repairer passes level -1 and no histogram, and its FileMetaData has no temperature set at that point, so repair's behavior is unchanged.

Safety, compatibility, limitations:

• No public API, option, or file format change. Behavior with max_open_files > 0, and in the OpenAndCompact worker, is unchanged.
• With max_open_files = -1, a properties request for a not-yet-open file now does the full table open on the caller's thread (e.g. index/filter blocks not cached in block cache, unique ID verification) instead of reading only the properties block. Total I/O goes down, but one GetPropertiesOfAllTables() call right after an async open can take longer, since it opens serially files that the background opener (using max_file_opening_threads) has not reached yet. Errors that fail a table open (e.g. corruption outside the properties block) now come back from these calls. With synchronous open the same error would have failed DB::Open, and with open_files_async it is also reported as a background error.
• NUMBER_DIRECT_LOAD_TABLE_PROPERTIES no longer counts these reads when max_open_files = -1.
• If GetPreparedFileInfoForExternalSstIngestion() is what opens a file, the reader now gets the file's level, histogram, temperature and L0 pinning size, matching how reads open it.

Requested by: Peter Dillinger (peterd)

Differential Revision: D127244681

[meta-codesync]

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

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 626.8s.

[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 1c73137

---

Summary

Well-designed optimization that eliminates redundant I/O for table properties when max_open_files = -1. The implementation correctly leverages FindTable's per-file coalescing mutex, matches the async opener's parameters (including prefetch_index_and_filter_in_cache = false), and properly handles the ReactiveVersionSet/OpenAndCompact special case. All callers pass correct level values.

High-severity findings (0):
No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Stale "REQUIRES: lock is held" comment — version_set.h:1125

• Issue: The updated GetTableProperties header comment (line 1125) still says "REQUIRES: lock is held", but the primary callers (GetPropertiesOfAllTables, GetPropertiesOfTablesInRange, InternalStats::HandleAggregatedTableProperties) release the DB mutex before calling. This was a pre-existing documentation bug, but the PR updates the comment block without fixing it, and the new behavior (doing I/O via FindTable) makes the comment more misleading — readers might think the DB mutex is held during file I/O.
• Root cause: Legacy comment that was never accurate for all callers. The Version is kept alive via Ref(), and FindTable has its own synchronization.
• Suggested fix: Update the comment to something like: "REQUIRES: this Version must be alive (Ref'd or current). Does not require the DB mutex." Or remove the REQUIRES line entirely since the actual contract is just that the Version is live.

M2. Error surface expansion underdocumented — version_set.cc:1981

• Issue: With max_open_files = -1 and open_files_async = true, GetPropertiesOfAllTables() previously succeeded even if non-properties blocks were corrupted (because it only read the properties block directly). Now it performs a full table open, so corruption in index blocks, unique ID verification failures, or other open-time checks can fail the call. The PR description documents this thoroughly, but the release note (unreleased_history/performance_improvements/table_properties_through_table_cache.md) does not mention the changed error behavior.
• Root cause: The release note focuses only on the performance improvement.
• Suggested fix: Consider adding a brief note about the error surface change to the release note, e.g.: "Errors that would previously only appear during reads (e.g., SST corruption outside the properties block) may now surface earlier through GetPropertiesOfAllTables() or compaction property lookups."

🟢 LOW / NIT

L1. GetAggregatedTableProperties also has stale "REQUIRES: lock is held" — version_set.h:1156

• Issue: Same stale comment pattern as M1. GetAggregatedTableProperties is called from InternalStats handlers which are registered with {false, ...} (not locked). Pre-existing issue, not introduced by this PR.
• Suggested fix: Fix alongside M1 if touching these comments.

L2. no_io default removal is good hygiene — table_cache.h:261

• Issue: Not a bug — positive observation. Removing the bool no_io = false default and requiring all callers to explicitly state their intent follows the CLAUDE.md guidance against defaulted parameters. The three callers now each explicitly pass the value.

Cross-Component Analysis

Context	Affected?	Analysis
WritePreparedTxnDB	No	GetTableProperties doesn't involve read_callback or transaction visibility
ReadOnly DB	Safe	Same path; table cache is in-memory, no writes to disk
ReactiveVersionSet (secondary)	Handled	Override returns false for trust_manifest_recovery_ (OpenAndCompact worker). Non-trust-manifest secondary with max_open_files=-1 correctly uses the cache path.
Dynamic max_open_files change	Safe	Brief inconsistency is benign: at worst, a file is opened into cache that might later be evicted, or a direct read is done when cache is now unbounded.
Async background opener	Compatible	Both paths pass prefetch_index_and_filter_in_cache=false (confirmed at db_impl_open.cc:3214). The async opener pins via FindTable; if properties path opened first, the opener finds it cached and pins it (table_cache.cc:337-351).
Compaction	Correct	Compaction::level(i) returns inputs_[i].level (actual LSM level). CompactionJob::Prepare passes each_level.level.

Assumption stress-test results:

• "Each file's metadata is read once" — Verified via FindTable's per-file loader_mutex_ coalescing.
• "prefetch=false matches the async opener" — Verified at db_impl_open.cc:3214.
• "No behavior change with max_open_files > 0" — Verified: OpensAllTableFiles() returns false, no_io=true, direct-read fallback preserved.
• "ReactiveVersionSet returns false for OpenAndCompact" — Verified: trust_manifest_recovery_ is set only for that path.

Positive Observations

• Removing no_io default follows CLAUDE.md guidance ("Avoid new defaulted parameters — Miss Spelling in README #1 trap on refactoring!"). All three callers now explicitly state intent.
• OpensAllTableFiles() virtual method is clean design — encapsulates the configuration question, allows ReactiveVersionSet override, avoids duplicating capacity checks.
• Test coverage is well-structured: parameterized over max_open_files=-1/10, tests before/after async open, verifies both counter values, uses sync points (no sleeps).
• Passing full open parameters (level, histogram, temperature, L0 meta pin size) ensures cached readers are identical to what the async opener would create.

---

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

---

Summary

Well-designed optimization that eliminates redundant I/O for table properties when max_open_files=-1. The implementation is clean: it correctly gates behavior via OpensAllTableFiles(), removes defaulted parameters to force callers to be explicit, and properly overrides for the ReactiveVersionSet + trust_manifest_recovery_ case. The new test uses sync points to verify correct behavior both before and after async open.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Header comment "REQUIRES: lock is held" is now misleading — version_set.h:1125

• Issue: Version::GetTableProperties() header comment says "REQUIRES: lock is held", but the main public caller DBImpl::GetPropertiesOfAllTables() (db_impl.cc:6109-6112) explicitly unlocks the DB mutex before calling this. With the new no_io=false path doing full table opens (potentially heavy I/O), the comment is more misleading than before. The code is actually safe because FindTable() uses its own loader_mutex_ for synchronization, but the stale contract comment could confuse future contributors.
• Root cause: Pre-existing inaccurate comment, now more likely to mislead since the function performs heavier work.
• Suggested fix: Update the comment to clarify the actual requirements: the Version must be ref-counted (alive), and the table cache provides its own synchronization for file opening. If the DB mutex IS required for some callers, document which ones and why.

M2. GetTableProperties no_io default removed but old TableCache signature in pre-diff header still visible — table_cache.h:261

• Issue: The diff removes the default value for no_io and adds three new non-defaulted parameters, which correctly forces all callers to be explicit. However, the declaration comment for @no_io at line 250-251 should be updated to reflect that the new parameters (level, file_read_hist, prefetch_index_and_filter_in_cache) are now also documented.
• Root cause: The new parameters have documentation at lines 252-254 in the diff, which is adequate. This is a minor nit.
• Suggested fix: No action required; the added comment at lines 252-254 is sufficient.

🟢 LOW / NIT

L1. Behavioral change: errors from full table opens now surface from properties queries — version_set.cc:1986

• Issue: With OpensAllTableFiles()=true, a GetPropertiesOfAllTables() call on a not-yet-opened file now does a full table open. If that open fails (corruption in index/filter blocks, unique ID mismatch, etc.), the error propagates to the caller. Previously, only the properties block was read (bypassing most table validation), so some corruption that would fail a full open was invisible here. This is explicitly documented in the PR description under "Safety, compatibility, limitations" and is arguably an improvement (surfacing errors earlier), but callers should be aware.
• Root cause: Intentional design decision, well-documented.
• Suggested fix: No change needed. The PR description covers this adequately.

L2. Version::GetTableProperties could be called with level=-1 if a caller is added later — version_set.cc:1977

• Issue: The new code calls cfd_->internal_stats()->GetFileReadHist(level) which indexes into a std::vector<HistogramImpl> of size num_levels. If level is ever negative, this is UB. Currently all callers pass valid levels (0..num_levels-1), and the repair path uses TableCache::GetTableProperties directly (not Version::GetTableProperties), but there's no assert on the level parameter.
• Root cause: Missing defensive check; all current callers are correct.
• Suggested fix: Consider adding assert(level >= 0 && level < storage_info_.num_levels_) at the top of Version::GetTableProperties.

L3. Release note is appropriate in scope — unreleased_history/performance_improvements/table_properties_through_table_cache.md

• Issue: The release note is concise and user-facing. Good.
• Root cause: N/A
• Suggested fix: N/A

Cross-Component Analysis

Context	Affected?	Analysis
WritePreparedTxnDB	No	Properties queries don't interact with read_callback visibility
ReadOnly DB	Yes (safe)	Uses same OpensAllTableFiles() gate; read-only DBs with max_open_files=-1 will also open through cache
CompactionService / Remote compaction	Yes (safe)	ReactiveVersionSet::OpensAllTableFiles() returns false when trust_manifest_recovery_=true, preserving existing behavior for OpenAndCompact workers
Repair	Yes (safe)	Repair calls TableCache::GetTableProperties directly, not Version::GetTableProperties. The diff correctly passes level=-1, file_read_hist=nullptr
User-defined timestamps	No	Properties access is independent of key comparison
Old snapshots	No	Properties access doesn't interact with snapshot visibility
FIFO / Universal compaction	No	Same GetTableProperties path, level values are valid

Positive Observations

• Eliminated defaulted parameters: Removing the no_io default and requiring all 4 parameters forces every caller to explicitly state their intent. This is a refactoring improvement per CLAUDE.md guidelines.
• OpensAllTableFiles() is cleanly virtual: The override in ReactiveVersionSet correctly handles the remote compaction worker case where opening files for properties would waste I/O and break byte-counting tests.
• Test coverage: The new OpenFilesAsyncTest::TableProperties test uses sync points to verify both the "before async open" and "after async open" scenarios, checking both NO_FILE_OPENS and NUMBER_DIRECT_LOAD_TABLE_PROPERTIES counters. It covers both max_open_files=-1 and max_open_files=10 via the parameterized test.
• Coalescing via loader_mutex_: The PR correctly leverages FindTable()'s existing per-file loader mutex to coalesce concurrent opens from properties queries and the background opener, avoiding duplicate I/O.
• prefetch_index_and_filter_in_cache=false: When opening a table just for properties, index/filter blocks are not prefetched into the block cache, which is appropriate since the background opener may use different prefetch settings and will handle this later.

---

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