diff --git a/HISTORY.md b/HISTORY.md index 7a19cf6d4..7e9532340 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -9,6 +9,7 @@ * Fix a bug in WAL tracking. Before this PR (#10087), calling `SyncWAL()` on the only WAL file of the db will not log the event in MANIFEST, thus allowing a subsequent `DB::Open` even if the WAL file is missing or corrupted. * Fix a bug that could return wrong results with `index_type=kHashSearch` and using `SetOptions` to change the `prefix_extractor`. * Fixed a bug in WAL tracking with wal_compression. WAL compression writes a kSetCompressionType record which is not associated with any sequence number. As result, WalManager::GetSortedWalsOfType() will skip these WALs and not return them to caller, e.g. Checkpoint, Backup, causing the operations to fail. +* Avoid a crash if the IDENTITY file is accidentally truncated to empty. A new DB ID will be written and generated on Open. ### Public API changes * Add new API GetUnixTime in Snapshot class which returns the unix time at which Snapshot is taken. diff --git a/db/db_basic_test.cc b/db/db_basic_test.cc index b41f48686..840f332dc 100644 --- a/db/db_basic_test.cc +++ b/db/db_basic_test.cc @@ -579,55 +579,84 @@ TEST_F(DBBasicTest, ManifestRollOver) { } while (ChangeCompactOptions()); } -TEST_F(DBBasicTest, IdentityAcrossRestarts1) { +TEST_F(DBBasicTest, IdentityAcrossRestarts) { + constexpr size_t kMinIdSize = 10; do { - std::string id1; - ASSERT_OK(db_->GetDbIdentity(id1)); - - Options options = CurrentOptions(); - Reopen(options); - std::string id2; - ASSERT_OK(db_->GetDbIdentity(id2)); - // id1 should match id2 because identity was not regenerated - ASSERT_EQ(id1.compare(id2), 0); - - std::string idfilename = IdentityFileName(dbname_); - ASSERT_OK(env_->DeleteFile(idfilename)); - Reopen(options); - std::string id3; - ASSERT_OK(db_->GetDbIdentity(id3)); - if (options.write_dbid_to_manifest) { - ASSERT_EQ(id1.compare(id3), 0); - } else { - // id1 should NOT match id3 because identity was regenerated - ASSERT_NE(id1.compare(id3), 0); + 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 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 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); + } + ASSERT_OK(ReadFileToString(env_, idfilename, &tmp)); + ASSERT_EQ(tmp, id5); } } while (ChangeCompactOptions()); } -TEST_F(DBBasicTest, IdentityAcrossRestarts2) { - do { - std::string id1; - ASSERT_OK(db_->GetDbIdentity(id1)); - - Options options = CurrentOptions(); - options.write_dbid_to_manifest = true; - Reopen(options); - std::string id2; - ASSERT_OK(db_->GetDbIdentity(id2)); - // id1 should match id2 because identity was not regenerated - ASSERT_EQ(id1.compare(id2), 0); - - std::string idfilename = IdentityFileName(dbname_); - ASSERT_OK(env_->DeleteFile(idfilename)); - Reopen(options); - std::string id3; - ASSERT_OK(db_->GetDbIdentity(id3)); - // id1 should NOT match id3 because identity was regenerated - ASSERT_EQ(id1, id3); - } while (ChangeCompactOptions()); -} - #ifndef ROCKSDB_LITE TEST_F(DBBasicTest, Snapshot) { env_->SetMockSleep(); diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h index 733a87a0c..27a3384ec 100644 --- a/db/db_impl/db_impl.h +++ b/db/db_impl/db_impl.h @@ -1479,8 +1479,10 @@ class DBImpl : public DB { virtual bool OwnTablesAndLogs() const { return true; } - // Set DB identity file, and write DB ID to manifest if necessary. - Status SetDBId(bool read_only, RecoveryContext* recovery_ctx); + // Setup DB identity file, and write DB ID to manifest if necessary. + Status SetupDBId(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); // REQUIRES: db mutex held when calling this function, but the db mutex can // be released and re-acquired. Db mutex will be held when the function diff --git a/db/db_impl/db_impl_files.cc b/db/db_impl/db_impl_files.cc index f73b178fc..596d3aa73 100644 --- a/db/db_impl/db_impl_files.cc +++ b/db/db_impl/db_impl_files.cc @@ -873,43 +873,55 @@ uint64_t PrecomputeMinLogNumberToKeep2PC( return min_log_number_to_keep; } -Status DBImpl::SetDBId(bool read_only, RecoveryContext* recovery_ctx) { +void DBImpl::SetDBId(std::string&& id, bool read_only, + RecoveryContext* recovery_ctx) { + 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); + assert(versions_->GetColumnFamilySet() != nullptr); + VersionEdit edit; + edit.SetDBId(db_id_); + versions_->db_id_ = db_id_; + recovery_ctx->UpdateVersionEdits( + versions_->GetColumnFamilySet()->GetDefault(), edit); + } +} + +Status DBImpl::SetupDBId(bool read_only, RecoveryContext* recovery_ctx) { Status s; - // Happens when immutable_db_options_.write_dbid_to_manifest is set to true - // the very first time. - if (db_id_.empty()) { - // Check for the IDENTITY file and create it if not there. - s = fs_->FileExists(IdentityFileName(dbname_), IOOptions(), nullptr); - // Typically Identity file is created in NewDB() and for some reason if - // it is no longer available then at this point DB ID is not in Identity - // file or Manifest. - if (s.IsNotFound()) { - // Create a new DB ID, saving to file only if allowed - if (read_only) { - db_id_ = env_->GenerateUniqueId(); - return Status::OK(); - } else { - s = SetIdentityFile(env_, dbname_); - if (!s.ok()) { - return 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; } - } else if (!s.ok()) { - assert(s.IsIOError()); - return s; - } - s = GetDbIdentityFromIdentityFile(&db_id_); - if (immutable_db_options_.write_dbid_to_manifest && s.ok()) { - assert(!read_only); - assert(recovery_ctx != nullptr); - assert(versions_->GetColumnFamilySet() != nullptr); - VersionEdit edit; - edit.SetDBId(db_id_); - versions_->db_id_ = db_id_; - recovery_ctx->UpdateVersionEdits( - versions_->GetColumnFamilySet()->GetDefault(), edit); - } - } else if (!read_only) { + } + } + 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); + } + // Persist it to IDENTITY file if allowed + if (!read_only) { s = SetIdentityFile(env_, dbname_, db_id_); } return s; diff --git a/db/db_impl/db_impl_open.cc b/db/db_impl/db_impl_open.cc index c87ac00ec..7c4852ab2 100644 --- a/db/db_impl/db_impl_open.cc +++ b/db/db_impl/db_impl_open.cc @@ -524,7 +524,8 @@ Status DBImpl::Recover( return s; } } - s = SetDBId(read_only, recovery_ctx); + s = SetupDBId(read_only, recovery_ctx); + ROCKS_LOG_INFO(immutable_db_options_.info_log, "DB ID: %s\n", db_id_.c_str()); if (s.ok() && !read_only) { s = DeleteUnreferencedSstFiles(recovery_ctx); }