Steps toward making IDENTITY file obsolete (#13019)

Summary:
* Set write_dbid_to_manifest=true by default
* Add new option write_identity_file (default true) that allows us to opt-in to future behavior without identity file
* Refactor related DB open code to minimize code duplication

_Recommend hiding whitespace changes for review_

Intended follow-up: add support to ldb for reading and even replacing the DB identity in the manifest. Could be a variant of `update_manifest` command or based on it.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/13019

Test Plan: unit tests and stress test updated for new functionality

Reviewed By: anand1976

Differential Revision: D62898229

Pulled By: pdillinger

fbshipit-source-id: c08b25cf790610b034e51a9de0dc78b921abbcf0

db/c.cc

@@ -4075,6 +4075,15 @@ void rocksdb_options_set_write_dbid_to_manifest(
   opt->rep.write_dbid_to_manifest = write_dbid_to_manifest;
 }
 
+unsigned char rocksdb_options_get_write_identity_file(rocksdb_options_t* opt) {
+  return opt->rep.write_identity_file;
+}
+
+void rocksdb_options_set_write_identity_file(
+    rocksdb_options_t* opt, unsigned char write_identity_file) {
+  opt->rep.write_identity_file = write_identity_file;
+}
+
 unsigned char rocksdb_options_get_track_and_verify_wals_in_manifest(
     rocksdb_options_t* opt) {
   return opt->rep.track_and_verify_wals_in_manifest;

db/c_test.c

@@ -772,6 +772,8 @@ int main(int argc, char** argv) {
   rocksdb_options_set_write_buffer_size(options, 100000);
   rocksdb_options_set_paranoid_checks(options, 1);
   rocksdb_options_set_max_open_files(options, 10);
+  /* Compatibility with how test was written */
+  rocksdb_options_set_write_dbid_to_manifest(options, 0);
 
   table_options = rocksdb_block_based_options_create();
   rocksdb_block_based_options_set_block_cache(table_options, cache);
@@ -962,15 +964,24 @@ int main(int argc, char** argv) {
     rocksdb_options_t* options_dbid_in_manifest = rocksdb_options_create();
     rocksdb_options_set_create_if_missing(options_dbid_in_manifest, 1);
 
+    rocksdb_options_set_write_dbid_to_manifest(options_dbid_in_manifest, false);
     unsigned char write_to_manifest =
         rocksdb_options_get_write_dbid_to_manifest(options_dbid_in_manifest);
     CheckCondition(!write_to_manifest);
     rocksdb_options_set_write_dbid_to_manifest(options_dbid_in_manifest, true);
-    CheckCondition(!write_to_manifest);
     write_to_manifest =
         rocksdb_options_get_write_dbid_to_manifest(options_dbid_in_manifest);
     CheckCondition(write_to_manifest);
 
+    rocksdb_options_set_write_identity_file(options_dbid_in_manifest, true);
+    unsigned char write_identity_file =
+        rocksdb_options_get_write_identity_file(options_dbid_in_manifest);
+    CheckCondition(write_identity_file);
+    rocksdb_options_set_write_identity_file(options_dbid_in_manifest, false);
+    write_identity_file =
+        rocksdb_options_get_write_identity_file(options_dbid_in_manifest);
+    CheckCondition(!write_identity_file);
+
     db = rocksdb_open(options_dbid_in_manifest, dbbackupname, &err);
     CheckNoError(err);
 

db/compaction/compaction_job_test.cc

@@ -545,15 +545,14 @@ class CompactionJobTestBase : public testing::Test {
     ASSERT_OK(s);
     db_options_.info_log = info_log;
 
-    versions_.reset(new VersionSet(
+    versions_.reset(
-        dbname_, &db_options_, env_options_, table_cache_.get(),
+        new VersionSet(dbname_, &db_options_, env_options_, table_cache_.get(),
-        &write_buffer_manager_, &write_controller_,
+                       &write_buffer_manager_, &write_controller_,
-        /*block_cache_tracer=*/nullptr, /*io_tracer=*/nullptr,
+                       /*block_cache_tracer=*/nullptr, /*io_tracer=*/nullptr,
-        /*db_id=*/"", /*db_session_id=*/"", /*daily_offpeak_time_utc=*/"",
+                       test::kUnitTestDbId, /*db_session_id=*/"",
-        /*error_handler=*/nullptr, /*read_only=*/false));
+                       /*daily_offpeak_time_utc=*/"",
+                       /*error_handler=*/nullptr, /*read_only=*/false));
     compaction_job_stats_.Reset();
-    ASSERT_OK(
-        SetIdentityFile(WriteOptions(), env_, dbname_, Temperature::kUnknown));
 
     VersionEdit new_db;
     new_db.SetLogNumber(0);

db/db_basic_test.cc

@@ -688,76 +688,100 @@ TEST_F(DBBasicTest, IdentityAcrossRestarts) {
   constexpr size_t kMinIdSize = 10;
   do {
     for (bool with_manifest : {false, true}) {
-      std::string idfilename = IdentityFileName(dbname_);
+      for (bool write_file : {false, true}) {
-      std::string id1, tmp;
+        std::string idfilename = IdentityFileName(dbname_);
-      ASSERT_OK(db_->GetDbIdentity(id1));
+        std::string id1, tmp;
-      ASSERT_GE(id1.size(), kMinIdSize);
+        ASSERT_OK(db_->GetDbIdentity(id1));
+        ASSERT_GE(id1.size(), kMinIdSize);
 
-      Options options = CurrentOptions();
+        Options options = CurrentOptions();
-      options.write_dbid_to_manifest = with_manifest;
+        options.write_dbid_to_manifest = with_manifest;
-      Reopen(options);
+        options.write_identity_file = true;  // initially
-      std::string id2;
+        Reopen(options);
-      ASSERT_OK(db_->GetDbIdentity(id2));
+        std::string id2;
-      // id2 should match id1 because identity was not regenerated
+        ASSERT_OK(db_->GetDbIdentity(id2));
-      ASSERT_EQ(id1, id2);
+        // id2 should match id1 because identity was not regenerated
-      ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
+        ASSERT_EQ(id1, id2);
-      ASSERT_EQ(tmp, id2);
+        ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
+        ASSERT_EQ(tmp, id2);
 
-      // Recover from deleted/missing IDENTITY
+        if (write_file) {
-      ASSERT_OK(env_->DeleteFile(idfilename));
+          // Recover from deleted/missing IDENTITY
-      Reopen(options);
+          ASSERT_OK(env_->DeleteFile(idfilename));
-      std::string id3;
+        } else {
-      ASSERT_OK(db_->GetDbIdentity(id3));
+          // Transition to no IDENTITY file
-      if (with_manifest) {
+          options.write_identity_file = false;
-        // id3 should match id1 because identity was restored from manifest
+          if (!with_manifest) {
-        ASSERT_EQ(id1, id3);
+            // Incompatible options, should fail
-      } else {
+            ASSERT_NOK(TryReopen(options));
-        // id3 should NOT match id1 because identity was regenerated
+            // Back to a usable config and continue
-        ASSERT_NE(id1, id3);
+            options.write_identity_file = true;
-        ASSERT_GE(id3.size(), kMinIdSize);
+            Reopen(options);
-      }
+            continue;
-      ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
+          }
-      ASSERT_EQ(tmp, id3);
+        }
+        Reopen(options);
+        std::string id3;
+        ASSERT_OK(db_->GetDbIdentity(id3));
+        if (with_manifest) {
+          // id3 should match id1 because identity was restored from manifest
+          ASSERT_EQ(id1, id3);
+        } else {
+          // id3 should NOT match id1 because identity was regenerated
+          ASSERT_NE(id1, id3);
+          ASSERT_GE(id3.size(), kMinIdSize);
+        }
+        if (write_file) {
+          ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
+          ASSERT_EQ(tmp, id3);
 
-      // Recover from truncated IDENTITY
+          // Recover from truncated IDENTITY
-      {
+          std::unique_ptr<WritableFile> w;
-        std::unique_ptr<WritableFile> w;
+          ASSERT_OK(env_->NewWritableFile(idfilename, &w, EnvOptions()));
-        ASSERT_OK(env_->NewWritableFile(idfilename, &w, EnvOptions()));
+          ASSERT_OK(w->Close());
-        ASSERT_OK(w->Close());
+        } else {
-      }
+          ASSERT_TRUE(env_->FileExists(idfilename).IsNotFound());
-      Reopen(options);
+        }
-      std::string id4;
+        Reopen(options);
-      ASSERT_OK(db_->GetDbIdentity(id4));
+        std::string id4;
-      if (with_manifest) {
+        ASSERT_OK(db_->GetDbIdentity(id4));
-        // id4 should match id1 because identity was restored from manifest
+        if (with_manifest) {
-        ASSERT_EQ(id1, id4);
+          // id4 should match id1 because identity was restored from manifest
-      } else {
+          ASSERT_EQ(id1, id4);
-        // id4 should NOT match id1 because identity was regenerated
+        } else {
-        ASSERT_NE(id1, id4);
+          // id4 should NOT match id1 because identity was regenerated
-        ASSERT_GE(id4.size(), kMinIdSize);
+          ASSERT_NE(id1, id4);
-      }
+          ASSERT_GE(id4.size(), kMinIdSize);
-      ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
+        }
-      ASSERT_EQ(tmp, id4);
+        std::string silly_id = "asdf123456789";
+        if (write_file) {
+          ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
+          ASSERT_EQ(tmp, id4);
 
-      // Recover from overwritten IDENTITY
+          // Recover from overwritten IDENTITY
-      std::string silly_id = "asdf123456789";
+          std::unique_ptr<WritableFile> w;
-      {
+          ASSERT_OK(env_->NewWritableFile(idfilename, &w, EnvOptions()));
-        std::unique_ptr<WritableFile> w;
+          ASSERT_OK(w->Append(silly_id));
-        ASSERT_OK(env_->NewWritableFile(idfilename, &w, EnvOptions()));
+          ASSERT_OK(w->Close());
-        ASSERT_OK(w->Append(silly_id));
+        } else {
-        ASSERT_OK(w->Close());
+          ASSERT_TRUE(env_->FileExists(idfilename).IsNotFound());
+        }
+        Reopen(options);
+        std::string id5;
+        ASSERT_OK(db_->GetDbIdentity(id5));
+        if (with_manifest) {
+          // id4 should match id1 because identity was restored from manifest
+          ASSERT_EQ(id1, id5);
+        } else {
+          ASSERT_EQ(id5, silly_id);
+        }
+        if (write_file) {
+          ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
+          ASSERT_EQ(tmp, id5);
+        } else {
+          ASSERT_TRUE(env_->FileExists(idfilename).IsNotFound());
+        }
       }
-      Reopen(options);
-      std::string id5;
-      ASSERT_OK(db_->GetDbIdentity(id5));
-      if (with_manifest) {
-        // id4 should match id1 because identity was restored from manifest
-        ASSERT_EQ(id1, id5);
-      } else {
-        ASSERT_EQ(id5, silly_id);
-      }
-      ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
-      ASSERT_EQ(tmp, id5);
     }
   } while (ChangeCompactOptions());
 }

db/db_impl/db_impl.h

@@ -1584,11 +1584,12 @@ class DBImpl : public DB {
 
   virtual bool OwnTablesAndLogs() const { return true; }
 
-  // Setup DB identity file, and write DB ID to manifest if necessary.
+  // Read/create DB identity file (as appropriate), and write DB ID to
+  // version_edit if provided.
   Status SetupDBId(const WriteOptions& write_options, bool read_only,
-                   RecoveryContext* recovery_ctx);
+                   bool is_new_db, VersionEdit* version_edit);
-  // Assign db_id_ and write DB ID to manifest if necessary.
+  // Assign db_id_ and write DB ID to version_edit if provided.
-  void SetDBId(std::string&& id, bool read_only, RecoveryContext* recovery_ctx);
+  void SetDBId(std::string&& id, bool read_only, VersionEdit* version_edit);
 
   // Collect a deduplicated collection of paths used by this DB, including
   // dbname_, DBOptions.db_paths, ColumnFamilyOptions.cf_paths.

db/db_impl/db_impl_files.cc

@@ -953,59 +953,60 @@ uint64_t PrecomputeMinLogNumberToKeep2PC(
 }
 
 void DBImpl::SetDBId(std::string&& id, bool read_only,
-                     RecoveryContext* recovery_ctx) {
+                     VersionEdit* version_edit) {
   assert(db_id_.empty());
   assert(!id.empty());
   db_id_ = std::move(id);
-  if (!read_only && immutable_db_options_.write_dbid_to_manifest) {
+  if (!read_only && version_edit) {
-    assert(recovery_ctx != nullptr);
+    assert(version_edit != nullptr);
     assert(versions_->GetColumnFamilySet() != nullptr);
-    VersionEdit edit;
+    version_edit->SetDBId(db_id_);
-    edit.SetDBId(db_id_);
     versions_->db_id_ = db_id_;
-    recovery_ctx->UpdateVersionEdits(
-        versions_->GetColumnFamilySet()->GetDefault(), edit);
   }
 }
 
 Status DBImpl::SetupDBId(const WriteOptions& write_options, bool read_only,
-                         RecoveryContext* recovery_ctx) {
+                         bool is_new_db, VersionEdit* version_edit) {
   Status s;
-  // Check for the IDENTITY file and create it if not there or
+  if (!is_new_db) {
-  // broken or not matching manifest
+    // Check for the IDENTITY file and create it if not there or
-  std::string db_id_in_file;
+    // broken or not matching manifest
-  s = fs_->FileExists(IdentityFileName(dbname_), IOOptions(), nullptr);
+    std::string db_id_in_file;
-  if (s.ok()) {
+    s = fs_->FileExists(IdentityFileName(dbname_), IOOptions(), nullptr);
-    s = GetDbIdentityFromIdentityFile(&db_id_in_file);
+    if (s.ok()) {
-    if (s.ok() && !db_id_in_file.empty()) {
+      s = GetDbIdentityFromIdentityFile(&db_id_in_file);
-      if (db_id_.empty()) {
+      if (s.ok() && !db_id_in_file.empty()) {
-        // Loaded from file and wasn't already known from manifest
+        if (db_id_.empty()) {
-        SetDBId(std::move(db_id_in_file), read_only, recovery_ctx);
+          // Loaded from file and wasn't already known from manifest
-        return s;
+          SetDBId(std::move(db_id_in_file), read_only, version_edit);
-      } else if (db_id_ == db_id_in_file) {
+          return s;
-        // Loaded from file and matches manifest
+        } else if (db_id_ == db_id_in_file) {
-        return s;
+          // Loaded from file and matches manifest
+          return s;
+        }
       }
     }
-  }
+    if (s.IsNotFound()) {
-  if (s.IsNotFound()) {
+      s = Status::OK();
-    s = Status::OK();
+    }
-  }
+    if (!s.ok()) {
-  if (!s.ok()) {
+      assert(s.IsIOError());
-    assert(s.IsIOError());
+      return s;
-    return s;
+    }
   }
   // Otherwise IDENTITY file is missing or no good.
   // Generate new id if needed
   if (db_id_.empty()) {
-    SetDBId(env_->GenerateUniqueId(), read_only, recovery_ctx);
+    SetDBId(env_->GenerateUniqueId(), read_only, version_edit);
   }
   // Persist it to IDENTITY file if allowed
-  if (!read_only) {
+  if (!read_only && immutable_db_options_.write_identity_file) {
     s = SetIdentityFile(write_options, env_, dbname_,
                         immutable_db_options_.metadata_write_temperature,
                         db_id_);
   }
+  // NOTE: an obsolete IDENTITY file with write_identity_file=false is handled
+  // elsewhere, so that it's only deleted after successful recovery
   return s;
 }
 

db/db_impl/db_impl_open.cc

@@ -289,28 +289,25 @@ Status DBImpl::ValidateOptions(const DBOptions& db_options) {
           "start_time and end_time cannot be the same");
     }
   }
+
+  if (!db_options.write_dbid_to_manifest && !db_options.write_identity_file) {
+    return Status::InvalidArgument(
+        "write_dbid_to_manifest and write_identity_file cannot both be false");
+  }
   return Status::OK();
 }
 
 Status DBImpl::NewDB(std::vector<std::string>* new_filenames) {
-  VersionEdit new_db;
+  VersionEdit new_db_edit;
   const WriteOptions write_options(Env::IOActivity::kDBOpen);
-  Status s = SetIdentityFile(write_options, env_, dbname_,
+  Status s = SetupDBId(write_options, /*read_only=*/false, /*is_new_db=*/true,
-                             immutable_db_options_.metadata_write_temperature);
+                       &new_db_edit);
   if (!s.ok()) {
     return s;
   }
-  if (immutable_db_options_.write_dbid_to_manifest) {
+  new_db_edit.SetLogNumber(0);
-    std::string temp_db_id;
+  new_db_edit.SetNextFile(2);
-    s = GetDbIdentityFromIdentityFile(&temp_db_id);
+  new_db_edit.SetLastSequence(0);
-    if (!s.ok()) {
-      return s;
-    }
-    new_db.SetDBId(temp_db_id);
-  }
-  new_db.SetLogNumber(0);
-  new_db.SetNextFile(2);
-  new_db.SetLastSequence(0);
 
   ROCKS_LOG_INFO(immutable_db_options_.info_log, "Creating manifest 1 \n");
   const std::string manifest = DescriptorFileName(dbname_, 1);
@@ -342,7 +339,7 @@ Status DBImpl::NewDB(std::vector<std::string>* new_filenames) {
         tmp_set.Contains(FileType::kDescriptorFile)));
     log::Writer log(std::move(file_writer), 0, false);
     std::string record;
-    new_db.EncodeTo(&record);
+    new_db_edit.EncodeTo(&record);
     s = log.AddRecord(write_options, record);
     if (s.ok()) {
       s = SyncManifest(&immutable_db_options_, write_options, log.file());
@@ -528,7 +525,7 @@ Status DBImpl::Recover(
     }
     assert(s.ok());
   }
-  assert(db_id_.empty());
+  assert(is_new_db || db_id_.empty());
   Status s;
   bool missing_table_file = false;
   if (!immutable_db_options_.best_efforts_recovery) {
@@ -674,7 +671,17 @@ Status DBImpl::Recover(
       }
     }
   }
-  s = SetupDBId(write_options, read_only, recovery_ctx);
+  if (is_new_db) {
+    // Already set up DB ID in NewDB
+  } else if (immutable_db_options_.write_dbid_to_manifest && recovery_ctx) {
+    VersionEdit edit;
+    s = SetupDBId(write_options, read_only, is_new_db, &edit);
+    recovery_ctx->UpdateVersionEdits(
+        versions_->GetColumnFamilySet()->GetDefault(), edit);
+  } else {
+    s = SetupDBId(write_options, read_only, is_new_db, nullptr);
+  }
+  assert(!s.ok() || !db_id_.empty());
   ROCKS_LOG_INFO(immutable_db_options_.info_log, "DB ID: %s\n", db_id_.c_str());
   if (s.ok() && !read_only) {
     s = MaybeUpdateNextFileNumber(recovery_ctx);
@@ -2132,6 +2139,13 @@ Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname,
     s = impl->LogAndApplyForRecovery(recovery_ctx);
   }
 
+  if (s.ok() && !impl->immutable_db_options_.write_identity_file) {
+    // On successful recovery, delete an obsolete IDENTITY file to avoid DB ID
+    // inconsistency
+    impl->env_->DeleteFile(IdentityFileName(impl->dbname_))
+        .PermitUncheckedError();
+  }
+
   if (s.ok() && impl->immutable_db_options_.persist_stats_to_disk) {
     impl->mutex_.AssertHeld();
     s = impl->InitPersistStatsColumnFamily();

db/flush_job_test.cc

@@ -142,12 +142,13 @@ class FlushJobTestBase : public testing::Test {
       column_families.emplace_back(cf_name, cf_options_);
     }
 
-    versions_.reset(new VersionSet(
+    versions_.reset(
-        dbname_, &db_options_, env_options_, table_cache_.get(),
+        new VersionSet(dbname_, &db_options_, env_options_, table_cache_.get(),
-        &write_buffer_manager_, &write_controller_,
+                       &write_buffer_manager_, &write_controller_,
-        /*block_cache_tracer=*/nullptr, /*io_tracer=*/nullptr,
+                       /*block_cache_tracer=*/nullptr, /*io_tracer=*/nullptr,
-        /*db_id=*/"", /*db_session_id=*/"", /*daily_offpeak_time_utc=*/"",
+                       test::kUnitTestDbId, /*db_session_id=*/"",
-        /*error_handler=*/nullptr, /*read_only=*/false));
+                       /*daily_offpeak_time_utc=*/"",
+                       /*error_handler=*/nullptr, /*read_only=*/false));
     EXPECT_OK(versions_->Recover(column_families, false));
   }
 

db/version_set.cc

@@ -5143,6 +5143,7 @@ VersionSet::VersionSet(
       fs_(_db_options->fs, io_tracer),
       clock_(_db_options->clock),
       dbname_(dbname),
+      db_id_(db_id),
       db_options_(_db_options),
       next_file_number_(2),
       manifest_file_number_(0),  // Filled by Recover()

db/version_set_test.cc

@@ -1282,6 +1282,8 @@ class VersionSetTestBase {
     assert(column_families != nullptr);
     assert(last_seqno != nullptr);
     assert(log_writer != nullptr);
+    ASSERT_OK(
+        SetIdentityFile(WriteOptions(), env_, dbname_, Temperature::kUnknown));
     VersionEdit new_db;
     if (db_options_.write_dbid_to_manifest) {
       DBOptions tmp_db_options;
@@ -1426,8 +1428,6 @@ class VersionSetTestBase {
   void NewDB() {
     SequenceNumber last_seqno;
     std::unique_ptr<log::Writer> log_writer;
-    ASSERT_OK(
-        SetIdentityFile(WriteOptions(), env_, dbname_, Temperature::kUnknown));
     PrepareManifest(&column_families_, &last_seqno, &log_writer);
     log_writer.reset();
     CreateCurrentFile();
@@ -3890,6 +3890,8 @@ class VersionSetTestMissingFiles : public VersionSetTestBase,
     assert(column_families != nullptr);
     assert(last_seqno != nullptr);
     assert(log_writer != nullptr);
+    ASSERT_OK(
+        SetIdentityFile(WriteOptions(), env_, dbname_, Temperature::kUnknown));
     const std::string manifest = DescriptorFileName(dbname_, 1);
     const auto& fs = env_->GetFileSystem();
     std::unique_ptr<WritableFileWriter> file_writer;

db_stress_tool/db_stress_common.h

@@ -260,6 +260,7 @@ DECLARE_bool(use_full_merge_v1);
 DECLARE_int32(sync_wal_one_in);
 DECLARE_bool(avoid_unnecessary_blocking_io);
 DECLARE_bool(write_dbid_to_manifest);
+DECLARE_bool(write_identity_file);
 DECLARE_bool(avoid_flush_during_recovery);
 DECLARE_uint64(max_write_batch_group_size_bytes);
 DECLARE_bool(level_compaction_dynamic_level_bytes);

db_stress_tool/db_stress_gflags.cc

@@ -993,6 +993,10 @@ DEFINE_bool(write_dbid_to_manifest,
             ROCKSDB_NAMESPACE::Options().write_dbid_to_manifest,
             "Write DB_ID to manifest");
 
+DEFINE_bool(write_identity_file,
+            ROCKSDB_NAMESPACE::Options().write_identity_file,
+            "Write DB_ID to IDENTITY file");
+
 DEFINE_bool(avoid_flush_during_recovery,
             ROCKSDB_NAMESPACE::Options().avoid_flush_during_recovery,
             "Avoid flush during recovery");

db_stress_tool/db_stress_test_base.cc

@@ -4044,6 +4044,7 @@ void InitializeOptionsFromFlags(
   options.manual_wal_flush = FLAGS_manual_wal_flush_one_in > 0 ? true : false;
   options.avoid_unnecessary_blocking_io = FLAGS_avoid_unnecessary_blocking_io;
   options.write_dbid_to_manifest = FLAGS_write_dbid_to_manifest;
+  options.write_identity_file = FLAGS_write_identity_file;
   options.avoid_flush_during_recovery = FLAGS_avoid_flush_during_recovery;
   options.max_write_batch_group_size_bytes =
       FLAGS_max_write_batch_group_size_bytes;

include/rocksdb/c.h

@@ -1644,6 +1644,10 @@ extern ROCKSDB_LIBRARY_API unsigned char
 rocksdb_options_get_write_dbid_to_manifest(rocksdb_options_t*);
 extern ROCKSDB_LIBRARY_API void rocksdb_options_set_write_dbid_to_manifest(
     rocksdb_options_t*, unsigned char);
+extern ROCKSDB_LIBRARY_API unsigned char
+rocksdb_options_get_write_identity_file(rocksdb_options_t*);
+extern ROCKSDB_LIBRARY_API void rocksdb_options_set_write_identity_file(
+    rocksdb_options_t*, unsigned char);
 
 extern ROCKSDB_LIBRARY_API unsigned char
 rocksdb_options_get_track_and_verify_wals_in_manifest(rocksdb_options_t*);

include/rocksdb/options.h

@@ -1380,16 +1380,24 @@ struct DBOptions {
   // ReadOptions::background_purge_on_iterator_cleanup.
   bool avoid_unnecessary_blocking_io = false;
 
-  // Historically DB ID has always been stored in Identity File in DB folder.
+  // The DB unique ID can be saved in the DB manifest (preferred, this option)
-  // If this flag is true, the DB ID is written to Manifest file in addition
+  // or an IDENTITY file (historical, deprecated), or both. If this option is
-  // to the Identity file. By doing this 2 problems are solved
+  // set to false (old behavior), then write_identity_file must be set to true.
-  // 1. We don't checksum the Identity file where as Manifest file is.
+  // The manifest is preferred because
-  // 2. Since the source of truth for DB is Manifest file DB ID will sit with
+  // 1. The IDENTITY file is not checksummed, so it is not as safe against
-  //    the source of truth. Previously the Identity file could be copied
+  //    corruption.
-  //    independent of Manifest and that can result in wrong DB ID.
+  // 2. The IDENTITY file may or may not be copied with the DB (e.g. not
-  // We recommend setting this flag to true.
+  //    copied by BackupEngine), so is not reliable for the provenance of a DB.
-  // Default: false
+  // This option might eventually be obsolete and removed as Identity files
-  bool write_dbid_to_manifest = false;
+  // are phased out.
+  bool write_dbid_to_manifest = true;
+
+  // It is expected that the Identity file will be obsoleted by recording
+  // DB ID in the manifest (see write_dbid_to_manifest). Setting this to true
+  // maintains the historical behavior of writing an Identity file, while
+  // setting to false is expected to be the future default. This option might
+  // eventually be obsolete and removed as Identity files are phased out.
+  bool write_identity_file = true;
 
   // The number of bytes to prefetch when reading the log. This is mostly useful
   // for reading a remotely located log, as it can save the number of

java/src/test/java/org/rocksdb/DBOptionsTest.java

@@ -793,9 +793,9 @@ public class DBOptionsTest {
   @Test
   public void writeDbidToManifest() {
     try (final DBOptions options = new DBOptions()) {
-      assertThat(options.writeDbidToManifest()).isEqualTo(false);
-      assertThat(options.setWriteDbidToManifest(true)).isEqualTo(options);
       assertThat(options.writeDbidToManifest()).isEqualTo(true);
+      assertThat(options.setWriteDbidToManifest(false)).isEqualTo(options);
+      assertThat(options.writeDbidToManifest()).isEqualTo(false);
     }
   }
 

java/src/test/java/org/rocksdb/OptionsTest.java

@@ -1379,9 +1379,9 @@ public class OptionsTest {
   @Test
   public void writeDbidToManifest() {
     try (final Options options = new Options()) {
-      assertThat(options.writeDbidToManifest()).isEqualTo(false);
-      assertThat(options.setWriteDbidToManifest(true)).isEqualTo(options);
       assertThat(options.writeDbidToManifest()).isEqualTo(true);
+      assertThat(options.setWriteDbidToManifest(false)).isEqualTo(options);
+      assertThat(options.writeDbidToManifest()).isEqualTo(false);
     }
   }
 

java/src/test/java/org/rocksdb/RocksDBTest.java

@@ -1641,7 +1641,7 @@ public class RocksDBTest {
       try (final RocksDB db = RocksDB.open(options, dbPath)) {
         final RocksDB.LiveFiles livefiles = db.getLiveFiles(true);
         assertThat(livefiles).isNotNull();
-        assertThat(livefiles.manifestFileSize).isEqualTo(70);
+        assertThat(livefiles.manifestFileSize).isEqualTo(116);
         assertThat(livefiles.files.size()).isEqualTo(3);
         assertThat(livefiles.files.get(0)).isEqualTo("/CURRENT");
         assertThat(livefiles.files.get(1)).isEqualTo("/MANIFEST-000005");

options/db_options.cc

@@ -407,6 +407,10 @@ static std::unordered_map<std::string, OptionTypeInfo>
          {offsetof(struct ImmutableDBOptions, write_dbid_to_manifest),
           OptionType::kBoolean, OptionVerificationType::kNormal,
           OptionTypeFlags::kNone}},
+        {"write_identity_file",
+         {offsetof(struct ImmutableDBOptions, write_identity_file),
+          OptionType::kBoolean, OptionVerificationType::kNormal,
+          OptionTypeFlags::kNone}},
         {"log_readahead_size",
          {offsetof(struct ImmutableDBOptions, log_readahead_size),
           OptionType::kSizeT, OptionVerificationType::kNormal,
@@ -772,6 +776,7 @@ ImmutableDBOptions::ImmutableDBOptions(const DBOptions& options)
       avoid_unnecessary_blocking_io(options.avoid_unnecessary_blocking_io),
       persist_stats_to_disk(options.persist_stats_to_disk),
       write_dbid_to_manifest(options.write_dbid_to_manifest),
+      write_identity_file(options.write_identity_file),
       log_readahead_size(options.log_readahead_size),
       file_checksum_gen_factory(options.file_checksum_gen_factory),
       best_efforts_recovery(options.best_efforts_recovery),
@@ -947,6 +952,8 @@ void ImmutableDBOptions::Dump(Logger* log) const {
                    persist_stats_to_disk);
   ROCKS_LOG_HEADER(log, "                Options.write_dbid_to_manifest: %d",
                    write_dbid_to_manifest);
+  ROCKS_LOG_HEADER(log, "                Options.write_identity_file: %d",
+                   write_identity_file);
   ROCKS_LOG_HEADER(
       log, "                Options.log_readahead_size: %" ROCKSDB_PRIszt,
       log_readahead_size);

options/db_options.h

@@ -89,6 +89,7 @@ struct ImmutableDBOptions {
   bool avoid_unnecessary_blocking_io;
   bool persist_stats_to_disk;
   bool write_dbid_to_manifest;
+  bool write_identity_file;
   size_t log_readahead_size;
   std::shared_ptr<FileChecksumGenFactory> file_checksum_gen_factory;
   bool best_efforts_recovery;

test_util/testutil.cc

@@ -773,4 +773,6 @@ void RegisterTestLibrary(const std::string& arg) {
     ObjectRegistry::Default()->AddLibrary("test", RegisterTestObjects, arg);
   }
 }
+
+const std::string kUnitTestDbId = "UnitTest";
 }  // namespace ROCKSDB_NAMESPACE::test

test_util/testutil.h

@@ -904,5 +904,7 @@ struct ReadOptionsNoIo : public ReadOptions {
 };
 extern const ReadOptionsNoIo kReadOptionsNoIo;
 
+extern const std::string kUnitTestDbId;
+
 }  // namespace test
 }  // namespace ROCKSDB_NAMESPACE

tools/db_crashtest.py

@@ -209,6 +209,7 @@ default_params = {
     "use_write_buffer_manager": lambda: random.randint(0, 1),
     "avoid_unnecessary_blocking_io": random.randint(0, 1),
     "write_dbid_to_manifest": random.randint(0, 1),
+    "write_identity_file": random.randint(0, 1),
     "avoid_flush_during_recovery": lambda: random.choice(
         [1 if t == 0 else 0 for t in range(0, 8)]
     ),
@@ -959,6 +960,12 @@ def finalize_and_sanitize(src_params):
         and dest_params.get("use_timed_put_one_in") == 1
     ):
         dest_params["use_timed_put_one_in"] = 3
+    if (
+        dest_params.get("write_dbid_to_manifest") == 0
+        and dest_params.get("write_identity_file") == 0
+    ):
+        # At least one must be true
+        dest_params["write_dbid_to_manifest"] = 1
     return dest_params
 
 

unreleased_history/behavior_changes/db_id.md

@@ -0,0 +1 @@
+* Set `write_dbid_to_manifest=true` by default. This means DB ID will now be preserved through backups, checkpoints, etc. by default. Also add `write_identity_file` option which can be set to false for anticipated future behavior.