diff --git a/include/rocksdb/utilities/backupable_db.h b/include/rocksdb/utilities/backupable_db.h index 1c810d7afe75bffb05784742967f19af65f30ba0..afff2c2ac74ffb9e68ff31530db3e46f9b458389 100644 --- a/include/rocksdb/utilities/backupable_db.h +++ b/include/rocksdb/utilities/backupable_db.h @@ -276,10 +276,14 @@ class BackupEngine { progress_callback); } - // deletes old backups, keeping latest num_backups_to_keep alive + // Deletes old backups, keeping latest num_backups_to_keep alive. + // See also DeleteBackup. virtual Status PurgeOldBackups(uint32_t num_backups_to_keep) = 0; - // deletes a specific backup + // Deletes a specific backup. If this operation (or PurgeOldBackups) + // is not completed due to crash, power failure, etc. the state + // will be cleaned up the next time you call DeleteBackup, + // PurgeOldBackups, or GarbageCollect. virtual Status DeleteBackup(BackupID backup_id) = 0; // Call this from another thread if you want to stop the backup @@ -287,8 +291,8 @@ class BackupEngine { // not wait for the backup to stop. // The backup will stop ASAP and the call to CreateNewBackup will // return Status::Incomplete(). It will not clean up after itself, but - // the state will remain consistent. The state will be cleaned up - // next time you create BackupableDB or RestoreBackupableDB. + // the state will remain consistent. The state will be cleaned up the + // next time you call CreateNewBackup or GarbageCollect. virtual void StopBackup() = 0; // Returns info about backups in backup_info @@ -323,12 +327,13 @@ class BackupEngine { // Returns Status::OK() if all checks are good virtual Status VerifyBackup(BackupID backup_id) = 0; - // Will delete all the files we don't need anymore - // It will do the full scan of the files/ directory and delete all the - // files that are not referenced. PurgeOldBackups() and DeleteBackup() - // will do a similar operation as needed to clean up from any incomplete - // deletions, so this function is not really needed if calling one of - // those. + // Will delete any files left over from incomplete creation or deletion of + // a backup. This is not normally needed as those operations also clean up + // after prior incomplete calls to the same kind of operation (create or + // delete). + // NOTE: This is not designed to delete arbitrary files added to the backup + // directory outside of BackupEngine, and clean-up is always subject to + // permissions on and availability of the underlying filesystem. virtual Status GarbageCollect() = 0; }; diff --git a/utilities/backupable/backupable_db.cc b/utilities/backupable/backupable_db.cc index 02f33da7ed405b91d4f18eeafc6ac2b1d77e4ed0..ab79abbe1860850c363375e3d56e6874e9141e42 100644 --- a/utilities/backupable/backupable_db.cc +++ b/utilities/backupable/backupable_db.cc @@ -764,7 +764,11 @@ Status BackupEngineImpl::CreateNewBackupWithMetadata( if (s.ok()) { // maybe last backup failed and left partial state behind, clean it up. // need to do this before updating backups_ such that a private dir - // named after new_backup_id will be cleaned up + // named after new_backup_id will be cleaned up. + // (If an incomplete new backup is followed by an incomplete delete + // of the latest full backup, then there could be more than one next + // id with a private dir, the last thing to be deleted in delete + // backup, but all will be cleaned up with a GarbageCollect.) s = GarbageCollect(); } else if (s.IsNotFound()) { // normal case, the new backup's private dir doesn't exist yet @@ -1572,53 +1576,57 @@ Status BackupEngineImpl::GarbageCollect() { "constrains how many backups the engine knows about"); } - if (options_.share_table_files && - options_.max_valid_backups_to_open == port::kMaxInt32) { + if (options_.max_valid_backups_to_open == port::kMaxInt32) { // delete obsolete shared files // we cannot do this when BackupEngine has `max_valid_backups_to_open` set // as those engines don't know about all shared files. - std::vector shared_children; - { - std::string shared_path; - if (options_.share_files_with_checksum) { - shared_path = GetAbsolutePath(GetSharedFileWithChecksumRel()); - } else { - shared_path = GetAbsolutePath(GetSharedFileRel()); - } - auto s = backup_env_->FileExists(shared_path); - if (s.ok()) { - s = backup_env_->GetChildren(shared_path, &shared_children); - } else if (s.IsNotFound()) { - s = Status::OK(); - } - if (!s.ok()) { - overall_status = s; - // Trying again later might work - might_need_garbage_collect_ = true; - } - } - for (auto& child : shared_children) { - std::string rel_fname; - if (options_.share_files_with_checksum) { - rel_fname = GetSharedFileWithChecksumRel(child); - } else { - rel_fname = GetSharedFileRel(child); - } - auto child_itr = backuped_file_infos_.find(rel_fname); - // if it's not refcounted, delete it - if (child_itr == backuped_file_infos_.end() || - child_itr->second->refs == 0) { - // this might be a directory, but DeleteFile will just fail in that - // case, so we're good - Status s = backup_env_->DeleteFile(GetAbsolutePath(rel_fname)); - ROCKS_LOG_INFO(options_.info_log, "Deleting %s -- %s", - rel_fname.c_str(), s.ToString().c_str()); - backuped_file_infos_.erase(rel_fname); + for (bool with_checksum : {false, true}) { + std::vector shared_children; + { + std::string shared_path; + if (with_checksum) { + shared_path = GetAbsolutePath(GetSharedFileWithChecksumRel()); + } else { + shared_path = GetAbsolutePath(GetSharedFileRel()); + } + auto s = backup_env_->FileExists(shared_path); + if (s.ok()) { + s = backup_env_->GetChildren(shared_path, &shared_children); + } else if (s.IsNotFound()) { + s = Status::OK(); + } if (!s.ok()) { + overall_status = s; // Trying again later might work might_need_garbage_collect_ = true; } } + for (auto& child : shared_children) { + if (child == "." || child == "..") { + continue; + } + std::string rel_fname; + if (with_checksum) { + rel_fname = GetSharedFileWithChecksumRel(child); + } else { + rel_fname = GetSharedFileRel(child); + } + auto child_itr = backuped_file_infos_.find(rel_fname); + // if it's not refcounted, delete it + if (child_itr == backuped_file_infos_.end() || + child_itr->second->refs == 0) { + // this might be a directory, but DeleteFile will just fail in that + // case, so we're good + Status s = backup_env_->DeleteFile(GetAbsolutePath(rel_fname)); + ROCKS_LOG_INFO(options_.info_log, "Deleting %s -- %s", + rel_fname.c_str(), s.ToString().c_str()); + backuped_file_infos_.erase(rel_fname); + if (!s.ok()) { + // Trying again later might work + might_need_garbage_collect_ = true; + } + } + } } } @@ -1634,6 +1642,9 @@ Status BackupEngineImpl::GarbageCollect() { } } for (auto& child : private_children) { + if (child == "." || child == "..") { + continue; + } // it's ok to do this when BackupEngine has `max_valid_backups_to_open` set // as the engine always knows all valid backup numbers. BackupID backup_id = 0; @@ -1650,6 +1661,9 @@ Status BackupEngineImpl::GarbageCollect() { std::vector subchildren; backup_env_->GetChildren(full_private_path, &subchildren); for (auto& subchild : subchildren) { + if (subchild == "." || subchild == "..") { + continue; + } Status s = backup_env_->DeleteFile(full_private_path + subchild); ROCKS_LOG_INFO(options_.info_log, "Deleting %s -- %s", (full_private_path + subchild).c_str(), diff --git a/utilities/backupable/backupable_db_test.cc b/utilities/backupable/backupable_db_test.cc index 19cff6648b860c77234aa90a20b06b1cd362edb8..010ef089739a018834f7b34d6ba56018bd333dc0 100644 --- a/utilities/backupable/backupable_db_test.cc +++ b/utilities/backupable/backupable_db_test.cc @@ -10,7 +10,9 @@ #if !defined(ROCKSDB_LITE) && !defined(OS_WIN) #include +#include #include +#include #include "db/db_impl/db_impl.h" #include "env/env_chroot.h" @@ -515,6 +517,15 @@ static void AssertEmpty(DB* db, int from, int to) { class BackupableDBTest : public testing::Test { public: + enum ShareOption { + kNoShare, + kShareNoChecksum, + kShareWithChecksum, + }; + + const std::vector kAllShareOptions = { + kNoShare, kShareNoChecksum, kShareWithChecksum}; + BackupableDBTest() { // set up files std::string db_chroot = test::PerThreadDBPath("backupable_db"); @@ -560,15 +571,8 @@ class BackupableDBTest : public testing::Test { return db; } - void OpenDBAndBackupEngineShareWithChecksum( - bool destroy_old_data = false, bool dummy = false, - bool /*share_table_files*/ = true, bool share_with_checksums = false) { - backupable_options_->share_files_with_checksum = share_with_checksums; - OpenDBAndBackupEngine(destroy_old_data, dummy, share_with_checksums); - } - void OpenDBAndBackupEngine(bool destroy_old_data = false, bool dummy = false, - bool share_table_files = true) { + ShareOption shared_option = kShareNoChecksum) { // reset all the defaults test_backup_env_->SetLimitWrittenFiles(1000000); test_db_env_->SetLimitWrittenFiles(1000000); @@ -583,7 +587,9 @@ class BackupableDBTest : public testing::Test { } db_.reset(db); backupable_options_->destroy_old_data = destroy_old_data; - backupable_options_->share_table_files = share_table_files; + backupable_options_->share_table_files = shared_option != kNoShare; + backupable_options_->share_files_with_checksum = + shared_option == kShareWithChecksum; BackupEngine* backup_engine; ASSERT_OK(BackupEngine::Open(test_db_env_.get(), *backupable_options_, &backup_engine)); @@ -1204,7 +1210,7 @@ TEST_F(BackupableDBTest, FailOverwritingBackups) { TEST_F(BackupableDBTest, NoShareTableFiles) { const int keys_iteration = 5000; - OpenDBAndBackupEngine(true, false, false); + OpenDBAndBackupEngine(true, false, kNoShare); for (int i = 0; i < 5; ++i) { FillDB(db_.get(), keys_iteration * i, keys_iteration * (i + 1)); ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), !!(i % 2))); @@ -1220,7 +1226,7 @@ TEST_F(BackupableDBTest, NoShareTableFiles) { // Verify that you can backup and restore with share_files_with_checksum on TEST_F(BackupableDBTest, ShareTableFilesWithChecksums) { const int keys_iteration = 5000; - OpenDBAndBackupEngineShareWithChecksum(true, false, true, true); + OpenDBAndBackupEngine(true, false, kShareWithChecksum); for (int i = 0; i < 5; ++i) { FillDB(db_.get(), keys_iteration * i, keys_iteration * (i + 1)); ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), !!(i % 2))); @@ -1238,7 +1244,7 @@ TEST_F(BackupableDBTest, ShareTableFilesWithChecksums) { TEST_F(BackupableDBTest, ShareTableFilesWithChecksumsTransition) { const int keys_iteration = 5000; // set share_files_with_checksum to false - OpenDBAndBackupEngineShareWithChecksum(true, false, true, false); + OpenDBAndBackupEngine(true, false, kShareNoChecksum); for (int i = 0; i < 5; ++i) { FillDB(db_.get(), keys_iteration * i, keys_iteration * (i + 1)); ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), true)); @@ -1251,65 +1257,107 @@ TEST_F(BackupableDBTest, ShareTableFilesWithChecksumsTransition) { } // set share_files_with_checksum to true and do some more backups - OpenDBAndBackupEngineShareWithChecksum(true, false, true, true); + OpenDBAndBackupEngine(false /* destroy_old_data */, false, + kShareWithChecksum); for (int i = 5; i < 10; ++i) { FillDB(db_.get(), keys_iteration * i, keys_iteration * (i + 1)); ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), true)); } CloseDBAndBackupEngine(); - for (int i = 0; i < 5; ++i) { - AssertBackupConsistency(i + 1, 0, keys_iteration * (i + 5 + 1), + // Verify first (about to delete) + AssertBackupConsistency(1, 0, keys_iteration, keys_iteration * 11); + + // For an extra challenge, make sure that GarbageCollect / DeleteBackup + // is OK even if we open without share_table_files + OpenDBAndBackupEngine(false /* destroy_old_data */, false, kNoShare); + backup_engine_->DeleteBackup(1); + backup_engine_->GarbageCollect(); + CloseDBAndBackupEngine(); + + // Verify rest (not deleted) + for (int i = 1; i < 10; ++i) { + AssertBackupConsistency(i + 1, 0, keys_iteration * (i + 1), keys_iteration * 11); } } +// This test simulates cleaning up after aborted or incomplete creation +// of a new backup. TEST_F(BackupableDBTest, DeleteTmpFiles) { - for (int cleanup_fn : {1, 2, 3}) { - for (bool shared_checksum : {false, true}) { - OpenDBAndBackupEngineShareWithChecksum( - false /* destroy_old_data */, false /* dummy */, - true /* share_table_files */, shared_checksum); + for (int cleanup_fn : {1, 2, 3, 4}) { + for (ShareOption shared_option : kAllShareOptions) { + OpenDBAndBackupEngine(false /* destroy_old_data */, false /* dummy */, + shared_option); + ASSERT_OK(backup_engine_->CreateNewBackup(db_.get())); + BackupID next_id = 1; + BackupID oldest_id = std::numeric_limits::max(); + { + std::vector backup_info; + backup_engine_->GetBackupInfo(&backup_info); + for (const auto& bi : backup_info) { + next_id = std::max(next_id, bi.backup_id + 1); + oldest_id = std::min(oldest_id, bi.backup_id); + } + } CloseDBAndBackupEngine(); - std::string shared_tmp = backupdir_; - if (shared_checksum) { - shared_tmp += "/shared_checksum"; - } else { - shared_tmp += "/shared"; + + // An aborted or incomplete new backup will always be in the next + // id (maybe more) + std::string next_private = "private/" + std::to_string(next_id); + + // NOTE: both shared and shared_checksum should be cleaned up + // regardless of how the backup engine is opened. + std::vector tmp_files_and_dirs; + for (const auto& dir_and_file : { + std::make_pair(std::string("shared"), + std::string(".00006.sst.tmp")), + std::make_pair(std::string("shared_checksum"), + std::string(".00007.sst.tmp")), + std::make_pair(next_private, std::string("00003.sst")), + }) { + std::string dir = backupdir_ + "/" + dir_and_file.first; + file_manager_->CreateDir(dir); + ASSERT_OK(file_manager_->FileExists(dir)); + + std::string file = dir + "/" + dir_and_file.second; + file_manager_->WriteToFile(file, "tmp"); + ASSERT_OK(file_manager_->FileExists(file)); + + tmp_files_and_dirs.push_back(file); } - shared_tmp += "/.00006.sst.tmp"; - std::string private_tmp_dir = backupdir_ + "/private/10"; - std::string private_tmp_file = private_tmp_dir + "/00003.sst"; - file_manager_->WriteToFile(shared_tmp, "tmp"); - file_manager_->CreateDir(private_tmp_dir); - file_manager_->WriteToFile(private_tmp_file, "tmp"); - ASSERT_OK(file_manager_->FileExists(private_tmp_dir)); - if (shared_checksum) { - OpenDBAndBackupEngineShareWithChecksum( - false /* destroy_old_data */, false /* dummy */, - true /* share_table_files */, true /* share_with_checksums */); - } else { - OpenDBAndBackupEngine(); + if (cleanup_fn != /*CreateNewBackup*/ 4) { + // This exists after CreateNewBackup because it's deleted then + // re-created. + tmp_files_and_dirs.push_back(backupdir_ + "/" + next_private); } + + OpenDBAndBackupEngine(false /* destroy_old_data */, false /* dummy */, + shared_option); // Need to call one of these explicitly to delete tmp files switch (cleanup_fn) { case 1: - (void)backup_engine_->GarbageCollect(); + ASSERT_OK(backup_engine_->GarbageCollect()); break; case 2: - (void)backup_engine_->DeleteBackup(1); + ASSERT_OK(backup_engine_->DeleteBackup(oldest_id)); break; case 3: - (void)backup_engine_->PurgeOldBackups(1); + ASSERT_OK(backup_engine_->PurgeOldBackups(1)); + break; + case 4: + // Does a garbage collect if it sees that next private dir exists + ASSERT_OK(backup_engine_->CreateNewBackup(db_.get())); break; default: assert(false); } CloseDBAndBackupEngine(); - ASSERT_EQ(Status::NotFound(), file_manager_->FileExists(shared_tmp)); - ASSERT_EQ(Status::NotFound(), - file_manager_->FileExists(private_tmp_file)); - ASSERT_EQ(Status::NotFound(), file_manager_->FileExists(private_tmp_dir)); + for (std::string file_or_dir : tmp_files_and_dirs) { + if (file_manager_->FileExists(file_or_dir) != Status::NotFound()) { + FAIL() << file_or_dir << " was expected to be deleted." << cleanup_fn; + } + } } } }