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: #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(
- dbname_, &db_options_, env_options_, table_cache_.get(),
- &write_buffer_manager_, &write_controller_,
- /*block_cache_tracer=*/nullptr, /*io_tracer=*/nullptr,
- /*db_id=*/"", /*db_session_id=*/"", /*daily_offpeak_time_utc=*/"",
- /*error_handler=*/nullptr, /*read_only=*/false));
+ versions_.reset(
+ new VersionSet(dbname_, &db_options_, env_options_, table_cache_.get(),
+ &write_buffer_manager_, &write_controller_,
+ /*block_cache_tracer=*/nullptr, /*io_tracer=*/nullptr,
+ test::kUnitTestDbId, /*db_session_id=*/"",
+ /*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_);
- std::string id1, tmp;
- ASSERT_OK(db_->GetDbIdentity(id1));
- ASSERT_GE(id1.size(), kMinIdSize);
-
- Options options = CurrentOptions();
- options.write_dbid_to_manifest = with_manifest;
- Reopen(options);
- std::string id2;
- ASSERT_OK(db_->GetDbIdentity(id2));
- // id2 should match id1 because identity was not regenerated
- ASSERT_EQ(id1, id2);
- ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
- ASSERT_EQ(tmp, id2);
-
- // Recover from deleted/missing IDENTITY
- ASSERT_OK(env_->DeleteFile(idfilename));
- 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);
- }
- ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
- ASSERT_EQ(tmp, id3);
-
- // Recover from truncated IDENTITY
- {
- std::unique_ptr<WritableFile> w;
- ASSERT_OK(env_->NewWritableFile(idfilename, &w, EnvOptions()));
- ASSERT_OK(w->Close());
- }
- Reopen(options);
- std::string id4;
- ASSERT_OK(db_->GetDbIdentity(id4));
- if (with_manifest) {
- // id4 should match id1 because identity was restored from manifest
- ASSERT_EQ(id1, id4);
- } else {
- // id4 should NOT match id1 because identity was regenerated
- ASSERT_NE(id1, id4);
- ASSERT_GE(id4.size(), kMinIdSize);
- }
- ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
- ASSERT_EQ(tmp, id4);
-
- // Recover from overwritten IDENTITY
- std::string silly_id = "asdf123456789";
- {
- std::unique_ptr<WritableFile> w;
- ASSERT_OK(env_->NewWritableFile(idfilename, &w, EnvOptions()));
- ASSERT_OK(w->Append(silly_id));
- ASSERT_OK(w->Close());
- }
- 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);
+ for (bool write_file : {false, true}) {
+ std::string idfilename = IdentityFileName(dbname_);
+ std::string id1, tmp;
+ ASSERT_OK(db_->GetDbIdentity(id1));
+ ASSERT_GE(id1.size(), kMinIdSize);
+
+ Options options = CurrentOptions();
+ options.write_dbid_to_manifest = with_manifest;
+ options.write_identity_file = true; // initially
+ Reopen(options);
+ std::string id2;
+ ASSERT_OK(db_->GetDbIdentity(id2));
+ // id2 should match id1 because identity was not regenerated
+ ASSERT_EQ(id1, id2);
+ ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
+ ASSERT_EQ(tmp, id2);
+
+ if (write_file) {
+ // Recover from deleted/missing IDENTITY
+ ASSERT_OK(env_->DeleteFile(idfilename));
+ } else {
+ // Transition to no IDENTITY file
+ options.write_identity_file = false;
+ if (!with_manifest) {
+ // Incompatible options, should fail
+ ASSERT_NOK(TryReopen(options));
+ // Back to a usable config and continue
+ options.write_identity_file = true;
+ Reopen(options);
+ continue;
+ }
+ }
+ 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
+ std::unique_ptr<WritableFile> w;
+ ASSERT_OK(env_->NewWritableFile(idfilename, &w, EnvOptions()));
+ ASSERT_OK(w->Close());
+ } else {
+ ASSERT_TRUE(env_->FileExists(idfilename).IsNotFound());
+ }
+ Reopen(options);
+ std::string id4;
+ ASSERT_OK(db_->GetDbIdentity(id4));
+ if (with_manifest) {
+ // id4 should match id1 because identity was restored from manifest
+ ASSERT_EQ(id1, id4);
+ } else {
+ // id4 should NOT match id1 because identity was regenerated
+ ASSERT_NE(id1, id4);
+ ASSERT_GE(id4.size(), kMinIdSize);
+ }
+ std::string silly_id = "asdf123456789";
+ if (write_file) {
+ ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
+ ASSERT_EQ(tmp, id4);
+
+ // Recover from overwritten IDENTITY
+ std::unique_ptr<WritableFile> w;
+ ASSERT_OK(env_->NewWritableFile(idfilename, &w, EnvOptions()));
+ ASSERT_OK(w->Append(silly_id));
+ ASSERT_OK(w->Close());
+ } 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);
+ }
+ if (write_file) {
+ ASSERT_OK(ReadFileToString(env_, idfilename, &tmp));
+ ASSERT_EQ(tmp, id5);
+ } else {
+ ASSERT_TRUE(env_->FileExists(idfilename).IsNotFound());
+ }
  }
- 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);
- // Assign db_id_ and write DB ID to manifest if necessary.
- void SetDBId(std::string&& id, bool read_only, RecoveryContext* recovery_ctx);
+ bool is_new_db, VersionEdit* version_edit);
+ // Assign db_id_ and write DB ID to version_edit if provided.
+ 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) {
- assert(recovery_ctx != nullptr);
+ if (!read_only && version_edit) {
+ assert(version_edit != nullptr);
  assert(versions_->GetColumnFamilySet() != nullptr);
- VersionEdit edit;
- edit.SetDBId(db_id_);
+ version_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
- // broken or not matching manifest
- std::string db_id_in_file;
- s = fs_->FileExists(IdentityFileName(dbname_), IOOptions(), nullptr);
- if (s.ok()) {
- s = GetDbIdentityFromIdentityFile(&db_id_in_file);
- if (s.ok() && !db_id_in_file.empty()) {
- if (db_id_.empty()) {
- // Loaded from file and wasn't already known from manifest
- SetDBId(std::move(db_id_in_file), read_only, recovery_ctx);
- return s;
- } else if (db_id_ == db_id_in_file) {
- // Loaded from file and matches manifest
- return s;
+ if (!is_new_db) {
+ // Check for the IDENTITY file and create it if not there or
+ // broken or not matching manifest
+ std::string db_id_in_file;
+ s = fs_->FileExists(IdentityFileName(dbname_), IOOptions(), nullptr);
+ if (s.ok()) {
+ s = GetDbIdentityFromIdentityFile(&db_id_in_file);
+ if (s.ok() && !db_id_in_file.empty()) {
+ if (db_id_.empty()) {
+ // Loaded from file and wasn't already known from manifest
+ SetDBId(std::move(db_id_in_file), read_only, version_edit);
+ return s;
+ } else if (db_id_ == db_id_in_file) {
+ // Loaded from file and matches manifest
+ return s;
+ }
  }
  }
- }
- if (s.IsNotFound()) {
- s = Status::OK();
- }
- if (!s.ok()) {
- assert(s.IsIOError());
- return s;
+  if (s.IsNotFound()) {
+  s = Status::OK();
+ }
+  if (!s.ok()) {
+  assert(s.IsIOError());
+  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;
 }