From 2b628ccfe575abebf22afa42f4273608a26a9539 Mon Sep 17 00:00:00 2001 From: Andrew Chang Date: Wed, 22 Jul 2026 16:01:44 -0700 Subject: [PATCH] Preserve fast_sst_open metadata across MANIFEST rotation (#14987) Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14987 The `fast_sst_open` optimization speeds up DB open times by persisting file metadata information in the MANIFEST. When MANIFESTs are rotated however, this metadata is currently getting dropped, since it is not fed through `VersionEdit::AddFile`. There were also a few additional missing `AddFile` call sites. Fix: Add an optional `file_open_metadata` to `AddFile`. Feed this through wherever we call `AddFile`. Reviewed By: anand1976 Differential Revision: D113265655 fbshipit-source-id: c8371d66f6555bb9c372eea095bfa30a5f56a88a --- db/db_etc2_test.cc | 125 ++++++++++++++++++++++++- db/db_impl/db_impl_compaction_flush.cc | 22 ++--- db/db_impl/db_impl_experimental.cc | 2 +- db/db_impl/db_impl_open.cc | 3 +- db/experimental.cc | 22 ++--- db/version_edit.h | 6 +- db/version_set.cc | 7 +- 7 files changed, 160 insertions(+), 27 deletions(-) diff --git a/db/db_etc2_test.cc b/db/db_etc2_test.cc index 14127b4225..ea3f5592cb 100644 --- a/db/db_etc2_test.cc +++ b/db/db_etc2_test.cc @@ -28,6 +28,7 @@ #include "rocksdb/wal_filter.h" #include "test_util/testutil.h" #include "util/defer.h" +#include "util/mutexlock.h" #include "util/random.h" #include "utilities/fault_injection_env.h" @@ -7902,7 +7903,10 @@ class FastOpenTestFS : public FileSystemWrapper { IODebugContext* dbg) override { if (file_opts.file_metadata != nullptr) { metadata_passed_on_open_.fetch_add(1, std::memory_order_relaxed); - last_metadata_received_ = *file_opts.file_metadata; + { + MutexLock l(&last_metadata_mu_); + last_metadata_received_ = *file_opts.file_metadata; + } } IOStatus s = target()->NewRandomAccessFile(fname, file_opts, result, dbg); if (s.ok()) { @@ -7921,18 +7925,21 @@ class FastOpenTestFS : public FileSystemWrapper { } std::string GetLastMetadataReceived() const { + MutexLock l(&last_metadata_mu_); return last_metadata_received_; } void ResetCounters() { metadata_retrieved_count_.store(0, std::memory_order_relaxed); metadata_passed_on_open_.store(0, std::memory_order_relaxed); + MutexLock l(&last_metadata_mu_); last_metadata_received_.clear(); } private: std::atomic metadata_retrieved_count_{0}; std::atomic metadata_passed_on_open_{0}; + mutable port::Mutex last_metadata_mu_; std::string last_metadata_received_; }; @@ -8274,6 +8281,122 @@ TEST_F(DBTest2, FastSstOpenDisableAfterMetadataPersisted) { ASSERT_OK(DestroyDB(test_dbname, options)); } +TEST_F(DBTest2, FastSstOpenMetadataPreservedAcrossManifestRotation) { + // Regression test: FileMetaData::file_open_metadata for unchanged live + // SSTs must survive a MANIFEST rotation. WriteCurrentStateToManifest + // snapshots live SSTs into a new MANIFEST; if it drops file_open_metadata + // for those unchanged SSTs, the fast_sst_open optimization is silently + // disabled for them until they are rewritten by flush/compaction/ingest. + auto test_fs = std::make_shared(env_->GetFileSystem()); + std::unique_ptr test_env(new CompositeEnvWrapper(env_, test_fs)); + + Options options; + options.env = test_env.get(); + options.fast_sst_open = true; + options.create_if_missing = true; + // Keep the pre-rotation SST from being rewritten by compaction. + options.disable_auto_compactions = true; + + std::string test_dbname = test::PerThreadDBPath("fast_open_manifest_rotate"); + ASSERT_OK(DestroyDB(test_dbname, options)); + + std::unique_ptr db; + ASSERT_OK(DB::Open(options, test_dbname, &db)); + + // Flush an SST; metadata is retrieved and persisted in the MANIFEST. + ASSERT_OK(db->Put(WriteOptions(), "key1", "value1")); + ASSERT_OK(db->Flush(FlushOptions())); + ASSERT_EQ(1, test_fs->GetMetadataRetrievedCount()); + + // Shrink max_manifest_file_size so the next MANIFEST write rotates and + // exercises WriteCurrentStateToManifest. + ASSERT_OK(db->SetDBOptions( + {{"max_manifest_file_size", "1"}, {"max_manifest_space_amp_pct", "0"}})); + + uint64_t manifest_before = + static_cast(db.get())->TEST_Current_Manifest_FileNo(); + + // Flush a second SST to trigger the rotation. SST #1 is unchanged, so + // the rotated MANIFEST snapshot must carry SST #1's file_open_metadata. + ASSERT_OK(db->Put(WriteOptions(), "key2", "value2")); + ASSERT_OK(db->Flush(FlushOptions())); + + uint64_t manifest_after = + static_cast(db.get())->TEST_Current_Manifest_FileNo(); + ASSERT_GT(manifest_after, manifest_before); + + db.reset(); + test_fs->ResetCounters(); + + // Reopen. Every live SST should be opened with file_open_metadata. + // Pre-fix: only SST #2 (added by the rotating edit) has metadata; SST #1 + // was stripped by the rotation snapshot, so the count would be 1. + ASSERT_OK(DB::Open(options, test_dbname, &db)); + + std::string value; + ASSERT_OK(db->Get(ReadOptions(), "key1", &value)); + ASSERT_EQ("value1", value); + ASSERT_OK(db->Get(ReadOptions(), "key2", &value)); + ASSERT_EQ("value2", value); + + ASSERT_EQ(2, test_fs->GetMetadataPassedOnOpenCount()); + + db.reset(); + ASSERT_OK(DestroyDB(test_dbname, options)); +} + +TEST_F(DBTest2, FastSstOpenMetadataPreservedAcrossTrivialMove) { + // Regression test: FileMetaData::file_open_metadata must survive a trivial + // move. Trivial move re-records an SST at a new level via DeleteFile + + // AddFile without physically rewriting the file. If AddFile drops + // file_open_metadata, the file loses its fast_sst_open advantage until + // it is next rewritten by real compaction/flush/ingest. + auto test_fs = std::make_shared(env_->GetFileSystem()); + std::unique_ptr test_env(new CompositeEnvWrapper(env_, test_fs)); + + Options options; + options.env = test_env.get(); + options.fast_sst_open = true; + options.create_if_missing = true; + options.disable_auto_compactions = true; + + std::string test_dbname = test::PerThreadDBPath("fast_open_trivial_move"); + ASSERT_OK(DestroyDB(test_dbname, options)); + + std::unique_ptr db; + ASSERT_OK(DB::Open(options, test_dbname, &db)); + + // Flush a single SST at L0; metadata is retrieved once and persisted. + ASSERT_OK(db->Put(WriteOptions(), "key1", "value1")); + ASSERT_OK(db->Flush(FlushOptions())); + ASSERT_EQ(1, test_fs->GetMetadataRetrievedCount()); + + // Manual compaction. With a single L0 file and empty lower levels, the + // compaction picker converts this into a trivial move (no physical + // rewrite), exercising PrepareTrivialMoveEdit. + ASSERT_OK(db->CompactRange(CompactRangeOptions(), nullptr, nullptr)); + // If the SST had been rewritten, metadata retrieval would have re-run on + // the new file. Unchanged count confirms this was a trivial move. + ASSERT_EQ(1, test_fs->GetMetadataRetrievedCount()); + + db.reset(); + test_fs->ResetCounters(); + + // Reopen. The trivially-moved SST should still carry its + // file_open_metadata and be opened via the fast_sst_open fast path. + // Pre-fix, the trivial move stripped it and this count would be 0. + ASSERT_OK(DB::Open(options, test_dbname, &db)); + + std::string value; + ASSERT_OK(db->Get(ReadOptions(), "key1", &value)); + ASSERT_EQ("value1", value); + + ASSERT_EQ(1, test_fs->GetMetadataPassedOnOpenCount()); + + db.reset(); + ASSERT_OK(DestroyDB(test_dbname, options)); +} + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { diff --git a/db/db_impl/db_impl_compaction_flush.cc b/db/db_impl/db_impl_compaction_flush.cc index f49dd1835e..d383924cd6 100644 --- a/db/db_impl/db_impl_compaction_flush.cc +++ b/db/db_impl/db_impl_compaction_flush.cc @@ -1623,16 +1623,16 @@ void DBImpl::PrepareTrivialMoveEdit(Compaction& c, LogBuffer* log_buffer, for (size_t i = 0; i < c.num_input_files(l); i++) { FileMetaData* f = c.input(l, i); c.edit()->DeleteFile(c.level(l), f->fd.GetNumber()); - c.edit()->AddFile(c.output_level(), f->fd.GetNumber(), f->fd.GetPathId(), - f->fd.GetFileSize(), f->smallest, f->largest, - f->fd.smallest_seqno, f->fd.largest_seqno, - f->marked_for_compaction, f->temperature, - f->oldest_blob_file_number, f->oldest_ancester_time, - f->file_creation_time, f->epoch_number, - f->file_checksum, f->file_checksum_func_name, - f->unique_id, f->compensated_range_deletion_size, - f->tail_size, f->user_defined_timestamps_persisted, - f->min_timestamp, f->max_timestamp); + c.edit()->AddFile( + c.output_level(), f->fd.GetNumber(), f->fd.GetPathId(), + f->fd.GetFileSize(), f->smallest, f->largest, f->fd.smallest_seqno, + f->fd.largest_seqno, f->marked_for_compaction, f->temperature, + f->oldest_blob_file_number, f->oldest_ancester_time, + f->file_creation_time, f->epoch_number, f->file_checksum, + f->file_checksum_func_name, f->unique_id, + f->compensated_range_deletion_size, f->tail_size, + f->user_defined_timestamps_persisted, f->min_timestamp, + f->max_timestamp, f->file_open_metadata); moved_bytes += static_cast(c.input(l, i)->fd.GetFileSize()); ROCKS_LOG_BUFFER( log_buffer, "[%s] Moved #%" PRIu64 " to level-%d %" PRIu64 " bytes\n", @@ -2209,7 +2209,7 @@ Status DBImpl::ReFitLevel(ColumnFamilyData* cfd, int level, int target_level) { f->file_checksum, f->file_checksum_func_name, f->unique_id, f->compensated_range_deletion_size, f->tail_size, f->user_defined_timestamps_persisted, f->min_timestamp, - f->max_timestamp); + f->max_timestamp, f->file_open_metadata); } ROCKS_LOG_DEBUG(immutable_db_options_.info_log, "[%s] Apply version edit:\n%s", cfd->GetName().c_str(), diff --git a/db/db_impl/db_impl_experimental.cc b/db/db_impl/db_impl_experimental.cc index bb6a9a2e40..1829a53c6f 100644 --- a/db/db_impl/db_impl_experimental.cc +++ b/db/db_impl/db_impl_experimental.cc @@ -145,7 +145,7 @@ Status DBImpl::PromoteL0(ColumnFamilyHandle* column_family, int target_level) { f->file_checksum_func_name, f->unique_id, f->compensated_range_deletion_size, f->tail_size, f->user_defined_timestamps_persisted, f->min_timestamp, - f->max_timestamp); + f->max_timestamp, f->file_open_metadata); } status = versions_->LogAndApply(cfd, read_options, write_options, &edit, diff --git a/db/db_impl/db_impl_open.cc b/db/db_impl/db_impl_open.cc index b0c6e7ccd1..63d60da800 100644 --- a/db/db_impl/db_impl_open.cc +++ b/db/db_impl/db_impl_open.cc @@ -683,7 +683,8 @@ Status DBImpl::Recover( f->file_checksum, f->file_checksum_func_name, f->unique_id, f->compensated_range_deletion_size, f->tail_size, f->user_defined_timestamps_persisted, - f->min_timestamp, f->max_timestamp); + f->min_timestamp, f->max_timestamp, + f->file_open_metadata); ROCKS_LOG_WARN(immutable_db_options_.info_log, "[%s] Moving #%" PRIu64 " from from_level-%d to from_level-%d %" PRIu64 diff --git a/db/experimental.cc b/db/experimental.cc index b6efc1a475..0eeee1e0fe 100644 --- a/db/experimental.cc +++ b/db/experimental.cc @@ -158,17 +158,17 @@ Status UpdateManifestForFilesState( // Current state inconsistent with manifest ++files_updated; edit.DeleteFile(level, number); - edit.AddFile(level, lf->fd.GetNumber(), lf->fd.GetPathId(), - lf->fd.GetFileSize(), lf->smallest, lf->largest, - lf->fd.smallest_seqno, lf->fd.largest_seqno, - lf->marked_for_compaction, temp, - lf->oldest_blob_file_number, - lf->oldest_ancester_time, lf->file_creation_time, - lf->epoch_number, lf->file_checksum, - lf->file_checksum_func_name, lf->unique_id, - lf->compensated_range_deletion_size, lf->tail_size, - lf->user_defined_timestamps_persisted, - lf->min_timestamp, lf->max_timestamp); + edit.AddFile( + level, lf->fd.GetNumber(), lf->fd.GetPathId(), + lf->fd.GetFileSize(), lf->smallest, lf->largest, + lf->fd.smallest_seqno, lf->fd.largest_seqno, + lf->marked_for_compaction, temp, lf->oldest_blob_file_number, + lf->oldest_ancester_time, lf->file_creation_time, + lf->epoch_number, lf->file_checksum, + lf->file_checksum_func_name, lf->unique_id, + lf->compensated_range_deletion_size, lf->tail_size, + lf->user_defined_timestamps_persisted, lf->min_timestamp, + lf->max_timestamp, lf->file_open_metadata); } } } else { diff --git a/db/version_edit.h b/db/version_edit.h index 4dad44f8a6..3edc9b790a 100644 --- a/db/version_edit.h +++ b/db/version_edit.h @@ -811,7 +811,8 @@ class VersionEdit { const uint64_t compensated_range_deletion_size, uint64_t tail_size, bool user_defined_timestamps_persisted, const std::string& min_timestamp = "", - const std::string& max_timestamp = "") { + const std::string& max_timestamp = "", + const std::string& file_open_metadata = "") { assert(smallest_seqno <= largest_seqno); new_files_.emplace_back( level, @@ -822,6 +823,9 @@ class VersionEdit { epoch_number, file_checksum, file_checksum_func_name, unique_id, compensated_range_deletion_size, tail_size, user_defined_timestamps_persisted, min_timestamp, max_timestamp)); + if (!file_open_metadata.empty()) { + new_files_.back().second.file_open_metadata = file_open_metadata; + } files_to_quarantine_.push_back(file); if (!HasLastSequence() || largest_seqno > GetLastSequence()) { SetLastSequence(largest_seqno); diff --git a/db/version_set.cc b/db/version_set.cc index acbbf5c0aa..1a2b697313 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -7677,6 +7677,11 @@ Status VersionSet::WriteCurrentStateToManifest( for (const auto& f : level_files) { assert(f); + // Preserve file_open_metadata so live SSTs still benefit from + // fast_sst_open after a MANIFEST rotation. Without this, the + // rotated MANIFEST snapshot drops kFileOpenMetadata for unchanged + // files and their fast-open advantage is lost until they are + // rewritten. edit.AddFile(level, f->fd.GetNumber(), f->fd.GetPathId(), f->fd.GetFileSize(), f->smallest, f->largest, f->fd.smallest_seqno, f->fd.largest_seqno, @@ -7686,7 +7691,7 @@ Status VersionSet::WriteCurrentStateToManifest( f->file_checksum_func_name, f->unique_id, f->compensated_range_deletion_size, f->tail_size, f->user_defined_timestamps_persisted, f->min_timestamp, - f->max_timestamp); + f->max_timestamp, f->file_open_metadata); } }