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

@@ -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<int> metadata_retrieved_count_{0};
   std::atomic<int> 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<FastOpenTestFS>(env_->GetFileSystem());
+  std::unique_ptr<Env> 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> 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<DBImpl*>(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<DBImpl*>(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<FastOpenTestFS>(env_->GetFileSystem());
+  std::unique_ptr<Env> 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> 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) {

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<size_t>(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(),

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,

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

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 {

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);

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);
         }
       }