From e3f396f1eaea52fdfb65f7248afd039abc3b275c Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Tue, 4 Mar 2014 17:02:25 -0800 Subject: [PATCH] Some fixes to BackupableDB Summary: (1) Report corruption if backup meta file has tailing data that was not read. This should fix: https://github.com/facebook/rocksdb/issues/81 (also, @sdong reported similar issue) (2) Don't use OS buffer when copying file to backup directory. We don't need the file in cache since we won't be reading it twice (3) Don't delete newer backups when somebody tries to backup the diverged DB (restore from older backup, add new data, try to backup). Rather, just fail the new backup. Test Plan: backupable_db_test Reviewers: ljin, dhruba, sdong Reviewed By: ljin CC: leveldb, sdong Differential Revision: https://reviews.facebook.net/D16287 --- HISTORY.md | 1 + include/utilities/backupable_db.h | 64 +++++++++--------- utilities/backupable/backupable_db.cc | 77 +++++++++++++--------- utilities/backupable/backupable_db_test.cc | 34 ++++++---- 4 files changed, 101 insertions(+), 75 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 4133dd2ad..0227580ad 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -8,6 +8,7 @@ * By default, checksums are verified on every read from database * Added is_manual_compaction to CompactionFilter::Context * Added "virtual void WaitForJoin() = 0" in class Env +* Removed BackupEngine::DeleteBackupsNewerThan() function ### New Features * If we find one truncated record at the end of the MANIFEST or WAL files, diff --git a/include/utilities/backupable_db.h b/include/utilities/backupable_db.h index ab3a1ed80..abf05978c 100644 --- a/include/utilities/backupable_db.h +++ b/include/utilities/backupable_db.h @@ -58,14 +58,13 @@ struct BackupableDBOptions { explicit BackupableDBOptions(const std::string& _backup_dir, Env* _backup_env = nullptr, bool _share_table_files = true, - Logger* _info_log = nullptr, - bool _sync = true, - bool _destroy_old_data = false) : - backup_dir(_backup_dir), - backup_env(_backup_env), - info_log(_info_log), - sync(_sync), - destroy_old_data(_destroy_old_data) { } + Logger* _info_log = nullptr, bool _sync = true, + bool _destroy_old_data = false) + : backup_dir(_backup_dir), + backup_env(_backup_env), + info_log(_info_log), + sync(_sync), + destroy_old_data(_destroy_old_data) {} }; typedef uint32_t BackupID; @@ -99,8 +98,6 @@ class BackupEngine { const std::string& wal_dir) = 0; virtual Status RestoreDBFromLatestBackup(const std::string& db_dir, const std::string& wal_dir) = 0; - - virtual void DeleteBackupsNewerThan(uint64_t sequence_number) = 0; }; // Stack your DB with BackupableDB to be able to backup the DB @@ -138,32 +135,33 @@ class BackupableDB : public StackableDB { // Use this class to access information about backups and restore from them class RestoreBackupableDB { - public: - RestoreBackupableDB(Env* db_env, const BackupableDBOptions& options); - ~RestoreBackupableDB(); + public: + RestoreBackupableDB(Env* db_env, const BackupableDBOptions& options); + ~RestoreBackupableDB(); - // Returns info about backups in backup_info - void GetBackupInfo(std::vector* backup_info); + // Returns info about backups in backup_info + void GetBackupInfo(std::vector* backup_info); - // restore from backup with backup_id - // IMPORTANT -- if options_.share_table_files == true and you restore DB - // from some backup that is not the latest, and you start creating new - // backups from the new DB, all the backups that were newer than the - // backup you restored from will be deleted - // - // Example: Let's say you have backups 1, 2, 3, 4, 5 and you restore 3. - // If you try creating a new backup now, old backups 4 and 5 will be deleted - // and new backup with ID 4 will be created. - Status RestoreDBFromBackup(BackupID backup_id, const std::string& db_dir, - const std::string& wal_dir); + // restore from backup with backup_id + // IMPORTANT -- if options_.share_table_files == true and you restore DB + // from some backup that is not the latest, and you start creating new + // backups from the new DB, they will probably fail + // + // Example: Let's say you have backups 1, 2, 3, 4, 5 and you restore 3. + // If you add new data to the DB and try creating a new backup now, the + // database will diverge from backups 4 and 5 and the new backup will fail. + // If you want to create new backup, you will first have to delete backups 4 + // and 5. + Status RestoreDBFromBackup(BackupID backup_id, const std::string& db_dir, + const std::string& wal_dir); - // restore from the latest backup - Status RestoreDBFromLatestBackup(const std::string& db_dir, - const std::string& wal_dir); - // deletes old backups, keeping latest num_backups_to_keep alive - Status PurgeOldBackups(uint32_t num_backups_to_keep); - // deletes a specific backup - Status DeleteBackup(BackupID backup_id); + // restore from the latest backup + Status RestoreDBFromLatestBackup(const std::string& db_dir, + const std::string& wal_dir); + // deletes old backups, keeping latest num_backups_to_keep alive + Status PurgeOldBackups(uint32_t num_backups_to_keep); + // deletes a specific backup + Status DeleteBackup(BackupID backup_id); private: BackupEngine* backup_engine_; diff --git a/utilities/backupable/backupable_db.cc b/utilities/backupable/backupable_db.cc index 89051f25a..422534470 100644 --- a/utilities/backupable/backupable_db.cc +++ b/utilities/backupable/backupable_db.cc @@ -46,8 +46,6 @@ class BackupEngineImpl : public BackupEngine { return RestoreDBFromBackup(latest_backup_id_, db_dir, wal_dir); } - void DeleteBackupsNewerThan(uint64_t sequence_number); - private: struct FileInfo { FileInfo(const std::string& fname, uint64_t sz, uint32_t checksum) @@ -185,6 +183,12 @@ class BackupEngineImpl : public BackupEngine { Env* db_env_; Env* backup_env_; + // directories + unique_ptr backup_directory_; + unique_ptr shared_directory_; + unique_ptr meta_directory_; + unique_ptr private_directory_; + static const size_t copy_file_buffer_size_ = 5 * 1024 * 1024LL; // 5MB }; @@ -203,11 +207,17 @@ BackupEngineImpl::BackupEngineImpl(Env* db_env, // create all the dirs we need backup_env_->CreateDirIfMissing(GetAbsolutePath()); + backup_env_->NewDirectory(GetAbsolutePath(), &backup_directory_); if (options_.share_table_files) { backup_env_->CreateDirIfMissing(GetAbsolutePath(GetSharedFileRel())); + backup_env_->NewDirectory(GetAbsolutePath(GetSharedFileRel()), + &shared_directory_); } backup_env_->CreateDirIfMissing(GetAbsolutePath(GetPrivateDirRel())); + backup_env_->NewDirectory(GetAbsolutePath(GetPrivateDirRel()), + &private_directory_); backup_env_->CreateDirIfMissing(GetBackupMetaDir()); + backup_env_->NewDirectory(GetBackupMetaDir(), &meta_directory_); std::vector backup_meta_files; backup_env_->GetChildren(GetBackupMetaDir(), &backup_meta_files); @@ -279,26 +289,6 @@ BackupEngineImpl::BackupEngineImpl(Env* db_env, BackupEngineImpl::~BackupEngineImpl() { LogFlush(options_.info_log); } -void BackupEngineImpl::DeleteBackupsNewerThan(uint64_t sequence_number) { - for (auto backup : backups_) { - if (backup.second.GetSequenceNumber() > sequence_number) { - Log(options_.info_log, - "Deleting backup %u because sequence number (%" PRIu64 - ") is newer than %" PRIu64 "", - backup.first, backup.second.GetSequenceNumber(), sequence_number); - backup.second.Delete(); - obsolete_backups_.push_back(backup.first); - } - } - for (auto ob : obsolete_backups_) { - backups_.erase(backups_.find(ob)); - } - auto itr = backups_.end(); - latest_backup_id_ = (itr == backups_.begin()) ? 0 : (--itr)->first; - PutLatestBackupFileContents(latest_backup_id_); // Ignore errors - GarbageCollection(false); -} - Status BackupEngineImpl::CreateNewBackup(DB* db, bool flush_before_backup) { Status s; std::vector live_files; @@ -348,9 +338,8 @@ Status BackupEngineImpl::CreateNewBackup(DB* db, bool flush_before_backup) { return Status::Corruption("Can't parse file name. This is very bad"); } // we should only get sst, manifest and current files here - assert(type == kTableFile || - type == kDescriptorFile || - type == kCurrentFile); + assert(type == kTableFile || type == kDescriptorFile || + type == kCurrentFile); // rules: // * if it's kTableFile, than it's shared @@ -394,6 +383,28 @@ Status BackupEngineImpl::CreateNewBackup(DB* db, bool flush_before_backup) { // install the newly created backup meta! (atomic) s = PutLatestBackupFileContents(new_backup_id); } + if (s.ok() && options_.sync) { + unique_ptr backup_private_directory; + backup_env_->NewDirectory( + GetAbsolutePath(GetPrivateFileRel(new_backup_id, false)), + &backup_private_directory); + if (backup_private_directory != nullptr) { + backup_private_directory->Fsync(); + } + if (private_directory_ != nullptr) { + private_directory_->Fsync(); + } + if (meta_directory_ != nullptr) { + meta_directory_->Fsync(); + } + if (shared_directory_ != nullptr) { + shared_directory_->Fsync(); + } + if (backup_directory_ != nullptr) { + backup_directory_->Fsync(); + } + } + if (!s.ok()) { // clean all the files we might have created Log(options_.info_log, "Backup failed -- %s", s.ToString().c_str()); @@ -591,6 +602,7 @@ Status BackupEngineImpl::CopyFile(const std::string& src, unique_ptr src_file; EnvOptions env_options; env_options.use_mmap_writes = false; + env_options.use_os_buffer = false; if (size != nullptr) { *size = 0; } @@ -706,6 +718,7 @@ Status BackupEngineImpl::CalculateChecksum(const std::string& src, Env* src_env, EnvOptions env_options; env_options.use_mmap_writes = false; + env_options.use_os_buffer = false; std::unique_ptr src_file; Status s = src_env->NewSequentialFile(src, &src_file, env_options); @@ -893,6 +906,9 @@ Status BackupEngineImpl::BackupMeta::LoadFromFile( uint64_t size; s = env_->GetFileSize(backup_dir + "/" + filename, &size); + if (!s.ok()) { + return s; + } if (line.empty()) { return Status::Corruption("File checksum is missing"); @@ -913,6 +929,11 @@ Status BackupEngineImpl::BackupMeta::LoadFromFile( files.emplace_back(filename, size, checksum_value); } + if (s.ok() && data.size() > 0) { + // file has to be read completely. if not, we count it as corruption + s = Status::Corruption("Tailing data in backup meta file"); + } + if (s.ok()) { for (const auto& file_info : files) { s = AddFile(file_info); @@ -968,11 +989,7 @@ Status BackupEngineImpl::BackupMeta::StoreToFile(bool sync) { BackupableDB::BackupableDB(DB* db, const BackupableDBOptions& options) : StackableDB(db), - backup_engine_(new BackupEngineImpl(db->GetEnv(), options)) { - if (options.share_table_files) { - backup_engine_->DeleteBackupsNewerThan(GetLatestSequenceNumber()); - } -} + backup_engine_(new BackupEngineImpl(db->GetEnv(), options)) {} BackupableDB::~BackupableDB() { delete backup_engine_; diff --git a/utilities/backupable/backupable_db_test.cc b/utilities/backupable/backupable_db_test.cc index ade2d954f..aaff224f7 100644 --- a/utilities/backupable/backupable_db_test.cc +++ b/utilities/backupable/backupable_db_test.cc @@ -709,27 +709,37 @@ TEST(BackupableDBTest, OnlineIntegrationTest) { CloseRestoreDB(); } -TEST(BackupableDBTest, DeleteNewerBackups) { +TEST(BackupableDBTest, FailOverwritingBackups) { + options_.write_buffer_size = 1024 * 1024 * 1024; // 1GB // create backups 1, 2, 3, 4, 5 OpenBackupableDB(true); for (int i = 0; i < 5; ++i) { FillDB(db_.get(), 100 * i, 100 * (i + 1)); - ASSERT_OK(db_->CreateNewBackup(!!(i % 2))); + ASSERT_OK(db_->CreateNewBackup(true)); + CloseBackupableDB(); + OpenBackupableDB(false); } CloseBackupableDB(); - // backup 3 is fine - AssertBackupConsistency(3, 0, 300, 500); - // this should delete backups 4 and 5 - OpenBackupableDB(); - CloseBackupableDB(); - // backups 4 and 5 don't exist + // restore 3 OpenRestoreDB(); - Status s = restore_db_->RestoreDBFromBackup(4, dbname_, dbname_); - ASSERT_TRUE(s.IsNotFound()); - s = restore_db_->RestoreDBFromBackup(5, dbname_, dbname_); - ASSERT_TRUE(s.IsNotFound()); + ASSERT_OK(restore_db_->RestoreDBFromBackup(3, dbname_, dbname_)); CloseRestoreDB(); + + OpenBackupableDB(false); + FillDB(db_.get(), 0, 300); + Status s = db_->CreateNewBackup(true); + // the new backup fails because new table files + // clash with old table files from backups 4 and 5 + // (since write_buffer_size is huge, we can be sure that + // each backup will generate only one sst file and that + // a file generated by a new backup is the same as + // sst file generated by backup 4) + ASSERT_TRUE(s.IsCorruption()); + ASSERT_OK(db_->DeleteBackup(4)); + ASSERT_OK(db_->DeleteBackup(5)); + // now, the backup can succeed + ASSERT_OK(db_->CreateNewBackup(true)); } TEST(BackupableDBTest, NoShareTableFiles) {