From 69976b4b6e49f7786142c7a1f385a5d435ca3da0 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Thu, 29 Oct 2026 21:14:27 -0700 Subject: [PATCH] Read table properties via TableCache if max_open_files=-1 Summary: X-link: https://github.com/facebook/rocksdb/pull/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 --- db/compaction/compaction.cc | 2 +- db/compaction/compaction_job.cc | 4 +- db/db_impl/db_impl.cc | 4 +- db/db_impl/db_impl_secondary.cc | 3 +- db/db_test.cc | 60 +++++++++++++++++++ db/repair.cc | 4 +- db/table_cache.cc | 11 ++-- db/table_cache.h | 7 ++- db/version_set.cc | 25 ++++---- db/version_set.h | 28 +++++++-- .../table_properties_through_table_cache.md | 1 + 11 files changed, 123 insertions(+), 26 deletions(-) create mode 100644 unreleased_history/performance_improvements/table_properties_through_table_cache.md diff --git a/db/compaction/compaction.cc b/db/compaction/compaction.cc index 02188d684668..729ce5107379 100644 --- a/db/compaction/compaction.cc +++ b/db/compaction/compaction.cc @@ -257,7 +257,7 @@ Status Compaction::InitInputTableProperties() { std::string file_name = TableFileName(immutable_options_.cf_paths, fmd->fd.GetNumber(), fmd->fd.GetPathId()); - s = input_version_->GetTableProperties(read_options, &tp, fmd, + s = input_version_->GetTableProperties(read_options, &tp, fmd, level(i), &file_name); if (s.ok()) { input_table_properties_[file_name] = tp; diff --git a/db/compaction/compaction_job.cc b/db/compaction/compaction_job.cc index f2d867a7bce7..330036cee393 100644 --- a/db/compaction/compaction_job.cc +++ b/db/compaction/compaction_job.cc @@ -341,8 +341,8 @@ void CompactionJob::Prepare( for (const auto& each_level : *c->inputs()) { for (const auto& fmd : each_level.files) { std::shared_ptr tp; - Status s = c->input_version()->GetTableProperties(read_options, &tp, - fmd, nullptr); + Status s = c->input_version()->GetTableProperties( + read_options, &tp, fmd, each_level.level, nullptr); if (s.ok()) { s = seqno_to_time_mapping_.DecodeFrom(tp->seqno_to_time_mapping); } diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index cd5fa9cff59b..683865754125 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -6841,7 +6841,9 @@ Status DBImpl::GetPreparedFileInfoForExternalSstIngestion( Status s = file_cfd->table_cache()->GetTableProperties( file_options_, read_options, file_cfd->internal_comparator(), *file_meta, &table_properties, file_version->GetMutableCFOptions(), - false /* no_io */); + /*no_io=*/false, file_level, + file_cfd->internal_stats()->GetFileReadHist(file_level), + /*prefetch_index_and_filter_in_cache=*/true); if (!s.ok()) { return s; } diff --git a/db/db_impl/db_impl_secondary.cc b/db/db_impl/db_impl_secondary.cc index 0c966438f311..a0b3d14e2623 100644 --- a/db/db_impl/db_impl_secondary.cc +++ b/db/db_impl/db_impl_secondary.cc @@ -1984,7 +1984,8 @@ Status DBImplSecondary::CompactWithoutInstallation( const ReadOptions read_options(Env::IOActivity::kCompaction); for (const auto& level_files : input_files) { for (FileMetaData* file_meta : level_files.files) { - version->MaybeInitializeFileMetaData(read_options, file_meta); + version->MaybeInitializeFileMetaData(read_options, file_meta, + level_files.level); } } } diff --git a/db/db_test.cc b/db/db_test.cc index e05bae88616e..41e7b009fb9d 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -8270,6 +8270,66 @@ TEST_P(OpenFilesAsyncTest, BeforeRead) { } } +TEST_P(OpenFilesAsyncTest, TableProperties) { + Options options = CurrentOptions(); + ASSERT_NO_FATAL_FAILURE(SetupData(options)); + + for (bool before_async_open : {true, false}) { + SCOPED_TRACE("before_async_open=" + std::to_string(before_async_open)); + if (before_async_open) { + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->LoadDependency({ + {"OpenFilesAsyncTest::TableProperties::AfterGet", + "DBImpl::BGWorkAsyncFileOpen::Start"}, + {"DBImpl::BGWorkAsyncFileOpen:Done", + "OpenFilesAsyncTest::TableProperties::BeforeCheck"}, + }); + } else { + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->LoadDependency({ + {"DBImpl::BGWorkAsyncFileOpen:Done", + "OpenFilesAsyncTest::TableProperties::BeforeCheck"}, + }); + } + + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing(); + + OpenTestDB(options); + + TablePropertiesCollection props; + ASSERT_OK(db_->GetPropertiesOfAllTables(&props)); + ASSERT_EQ(props.size(), num_flushes_); + + if (before_async_open) { + if (max_open_files_ == -1) { + // Opened through the table cache, as the async opener would + EXPECT_EQ( + TestGetTickerCount(options, NUMBER_DIRECT_LOAD_TABLE_PROPERTIES), + 0); + EXPECT_EQ(TestGetTickerCount(options, NO_FILE_OPENS), num_flushes_); + } else { + // Read directly from the files, without using up table cache capacity + EXPECT_EQ( + TestGetTickerCount(options, NUMBER_DIRECT_LOAD_TABLE_PROPERTIES), + num_flushes_); + EXPECT_EQ(TestGetTickerCount(options, NO_FILE_OPENS), 0); + } + } + + TEST_SYNC_POINT("OpenFilesAsyncTest::TableProperties::AfterGet"); + TEST_SYNC_POINT("OpenFilesAsyncTest::TableProperties::BeforeCheck"); + + // Each file is opened only once in total + EXPECT_EQ(TestGetTickerCount(options, NO_FILE_OPENS), num_flushes_); + if (max_open_files_ == -1) { + EXPECT_EQ( + TestGetTickerCount(options, NUMBER_DIRECT_LOAD_TABLE_PROPERTIES), 0); + } + + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing(); + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->ClearAllCallBacks(); + Close(); + } +} + // For modern DBs (manifest carries file_creation_time), // GetCreationTimeOfOldestFile returns the real value without waiting for async // file open. diff --git a/db/repair.cc b/db/repair.cc index 37c01bc2a7b7..2c0e9833db96 100644 --- a/db/repair.cc +++ b/db/repair.cc @@ -552,7 +552,9 @@ class Repairer { // TODO: plumb Env::IOActivity, Env::IOPriority const ReadOptions read_options; status = table_cache_->GetTableProperties( - file_options_, read_options, icmp_, t->meta, &props, default_mopts_); + file_options_, read_options, icmp_, t->meta, &props, default_mopts_, + /*no_io=*/false, /*level=*/-1, /*file_read_hist=*/nullptr, + /*prefetch_index_and_filter_in_cache=*/true); } if (status.ok()) { auto s = diff --git a/db/table_cache.cc b/db/table_cache.cc index 27da49eb92d8..b05f9d0cbdd5 100644 --- a/db/table_cache.cc +++ b/db/table_cache.cc @@ -731,12 +731,15 @@ Status TableCache::GetTableProperties( const InternalKeyComparator& internal_comparator, const FileMetaData& file_meta, std::shared_ptr* properties, - const MutableCFOptions& mutable_cf_options, bool no_io) { + const MutableCFOptions& mutable_cf_options, bool no_io, int level, + HistogramImpl* file_read_hist, bool prefetch_index_and_filter_in_cache) { TypedHandle* table_handle = nullptr; TableReader* table = nullptr; - Status s = - FindTable(read_options, file_options, internal_comparator, file_meta, - &table_handle, mutable_cf_options, &table, no_io); + Status s = FindTable( + read_options, file_options, internal_comparator, file_meta, &table_handle, + mutable_cf_options, &table, no_io, file_read_hist, /*skip_filters=*/false, + level, prefetch_index_and_filter_in_cache, + MaxFileSizeForL0MetaPin(mutable_cf_options), file_meta.temperature); if (!s.ok()) { return s; } diff --git a/db/table_cache.h b/db/table_cache.h index e2dd3427cd52..12f95318c5aa 100644 --- a/db/table_cache.h +++ b/db/table_cache.h @@ -249,6 +249,9 @@ class TableCache { // Get the table properties of a given table. // @no_io: indicates if we should load table to the cache if it is not present // in table cache yet. + // @level, @file_read_hist, @prefetch_index_and_filter_in_cache: as for + // FindTable(), applied if the table is opened here. Pass -1 and + // nullptr if the table's level is unknown. // @returns: `properties` will be reset on success. Please note that we will // return Status::Incomplete() if table is not present in cache and // we set `no_io` to be true. @@ -258,7 +261,9 @@ class TableCache { const FileMetaData& file_meta, std::shared_ptr* properties, const MutableCFOptions& mutable_cf_options, - bool no_io = false); + bool no_io, int level, + HistogramImpl* file_read_hist, + bool prefetch_index_and_filter_in_cache); Status ApproximateKeyAnchors(const ReadOptions& ro, const InternalKeyComparator& internal_comparator, diff --git a/db/version_set.cc b/db/version_set.cc index 1e7cc68d3cae..9100542e972e 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1970,20 +1970,25 @@ void LevelIterator::InitFileIterator(size_t new_file_index) { Status Version::GetTableProperties(const ReadOptions& read_options, std::shared_ptr* tp, - const FileMetaData* file_meta, + const FileMetaData* file_meta, int level, const std::string* fname) const { auto* table_cache = cfd_->table_cache(); const auto& ioptions = cfd_->ioptions(); + // If every table file gets opened anyway, open this one now (coalesced with + // any concurrent open) rather than reading its metadata an extra time. + const bool open_table = vset_->OpensAllTableFiles(); Status s = table_cache->GetTableProperties( file_options_, read_options, cfd_->internal_comparator(), *file_meta, tp, - mutable_cf_options_, true /* no io */); + mutable_cf_options_, /*no_io=*/!open_table, level, + cfd_->internal_stats()->GetFileReadHist(level), + /*prefetch_index_and_filter_in_cache=*/false); if (s.ok()) { return s; } // We only ignore error type `Incomplete` since it's by design that we // disallow table when it's not in table cache. - if (!s.IsIncomplete()) { + if (open_table || !s.IsIncomplete()) { return s; } @@ -2107,8 +2112,8 @@ Status Version::GetPropertiesOfAllTables(const ReadOptions& read_options, // 1. If the table is already present in table cache, load table // properties from there. std::shared_ptr table_properties; - Status s = - GetTableProperties(read_options, &table_properties, file_meta, &fname); + Status s = GetTableProperties(read_options, &table_properties, file_meta, + level, &fname); if (s.ok()) { props->insert({fname, table_properties}); } else { @@ -2139,7 +2144,7 @@ Status Version::GetPropertiesOfTablesInRange( // properties from there. std::shared_ptr table_properties; Status s = GetTableProperties(read_options, &table_properties, - file_meta, &fname); + file_meta, level, &fname); if (s.ok()) { props->insert({fname, table_properties}); } else { @@ -3350,12 +3355,12 @@ void Version::PrepareAppend(const ReadOptions& read_options, } bool Version::MaybeInitializeFileMetaData(const ReadOptions& read_options, - FileMetaData* file_meta) { + FileMetaData* file_meta, int level) { if (file_meta->init_stats_from_file || file_meta->compensated_file_size > 0) { return false; } std::shared_ptr tp; - Status s = GetTableProperties(read_options, &tp, file_meta); + Status s = GetTableProperties(read_options, &tp, file_meta, level); file_meta->init_stats_from_file = true; if (!s.ok()) { ROCKS_LOG_ERROR(vset_->db_options_->info_log, @@ -3425,7 +3430,7 @@ void Version::UpdateAccumulatedStats(const ReadOptions& read_options) { level < storage_info_.num_levels_ && init_count < kMaxInitCount; ++level) { for (auto* file_meta : storage_info_.files_[level]) { - if (MaybeInitializeFileMetaData(read_options, file_meta)) { + if (MaybeInitializeFileMetaData(read_options, file_meta, level)) { // each FileMeta will be initialized only once. storage_info_.UpdateAccumulatedStats(file_meta); // when option "max_open_files" is -1, all the file metadata has @@ -3451,7 +3456,7 @@ void Version::UpdateAccumulatedStats(const ReadOptions& read_options) { for (int i = static_cast(storage_info_.files_[level].size()) - 1; storage_info_.accumulated_raw_value_size_ == 0 && i >= 0; --i) { if (MaybeInitializeFileMetaData(read_options, - storage_info_.files_[level][i])) { + storage_info_.files_[level][i], level)) { storage_info_.UpdateAccumulatedStats(storage_info_.files_[level][i]); } } diff --git a/db/version_set.h b/db/version_set.h index f91e58e958b2..9bd516e4168c 100644 --- a/db/version_set.h +++ b/db/version_set.h @@ -1124,12 +1124,17 @@ class Version { // REQUIRES: lock is held // On success, "tp" will contains the table properties of the file - // specified in "file_meta". If the file name of "file_meta" is - // known ahead, passing it by a non-null "fname" can save a - // file-name conversion. + // specified in "file_meta", which is at "level" of this version. If the + // file name of "file_meta" is known ahead, passing it by a non-null "fname" + // can save a file-name conversion. + // + // If VersionSet::OpensAllTableFiles(), this opens the file's TableReader + // through the table cache if it is not already open. Otherwise, the + // properties block is read directly from the file unless its TableReader is + // already in the table cache. Status GetTableProperties(const ReadOptions& read_options, std::shared_ptr* tp, - const FileMetaData* file_meta, + const FileMetaData* file_meta, int level, const std::string* fname = nullptr) const; // On success, *props will be populated with all SSTables' table properties. @@ -1228,7 +1233,7 @@ class Version { // fields of file_meta from its associated TableProperties. // Returns true if it does initialize FileMetaData. bool MaybeInitializeFileMetaData(const ReadOptions& read_options, - FileMetaData* file_meta); + FileMetaData* file_meta, int level); // Update the accumulated stats associated with the current version. // This accumulated stats will be used in compaction. @@ -1546,6 +1551,13 @@ class VersionSet { virtual void ApplyCommittedOptionsFileNumber(uint64_t number); void ApplyLegacyOptionsFileNumber(uint64_t number); + // True if every live table file is opened and kept open in the table cache + // regardless of reads (during recovery and LogAndApply, or by the + // open_files_async background opener), as with max_open_files == -1. + virtual bool OpensAllTableFiles() const { + return table_cache_->GetCapacity() >= TableCache::kInfiniteCapacity; + } + uint64_t pending_manifest_file_number() const { return pending_manifest_file_number_; } @@ -2192,6 +2204,12 @@ class ReactiveVersionSet : public VersionSet { // or blob files. Only used by the DB::OpenAndCompact remote-compaction path. void SetTrustManifestRecovery(bool v) { trust_manifest_recovery_ = v; } + // With trusted MANIFEST recovery, only the table files that are read get + // opened. + bool OpensAllTableFiles() const override { + return !trust_manifest_recovery_ && VersionSet::OpensAllTableFiles(); + } + // Returns the column family's log number as of the Version currently // installed for it, i.e. the log number that the MANIFEST records that // Version was built from had put in effect. Data written to WALs older than diff --git a/unreleased_history/performance_improvements/table_properties_through_table_cache.md b/unreleased_history/performance_improvements/table_properties_through_table_cache.md new file mode 100644 index 000000000000..cc71fcb00b9b --- /dev/null +++ b/unreleased_history/performance_improvements/table_properties_through_table_cache.md @@ -0,0 +1 @@ +With `max_open_files = -1`, getting the table properties of an SST file that is not yet open (e.g. with `open_files_async`), as in `GetPropertiesOfAllTables()` or compaction, now opens it through the table cache, rather than reading its properties separately and opening it again later.