From a1b54848110ae9d85df05e27afe470ff0f369196 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Fri, 21 Aug 2020 08:14:54 -0700 Subject: [PATCH] Work around a backup bug with DB custom checksums (#7294) Summary: On a read-write DB configured with DBOptions::file_checksum_gen_factory, BackupEngine::CreateNewBackup can fail intermittently, with non-OK status. This is due to a race between GetLiveFiles and GetLiveFilesChecksumInfo in creating backups. For patching 6.12 release (as this commit is intended for, except this is a forward-merged version), we can simply treat files for which we falsely failed to get checksum info as legacy files lacking checksum info. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7294 Test Plan: unit test reproducer included Reviewed By: ajkr Differential Revision: D23253489 Pulled By: pdillinger fbshipit-source-id: 9e4945dad120b776ad3e753be10b962f61f28e14 --- HISTORY.md | 1 + utilities/backupable/backupable_db_test.cc | 32 ++++++++++++++++++++++ utilities/checkpoint/checkpoint_impl.cc | 11 +++++++- 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/HISTORY.md b/HISTORY.md index 2acc39a7a..6507a9332 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -5,6 +5,7 @@ * Fix a possible corruption to the LSM state (overlapping files within a level) when a `CompactRange()` for refitting levels (`CompactRangeOptions::change_level == true`) and another manual compaction are executed in parallel. * Sanitize `recycle_log_file_num` to zero when the user attempts to enable it in combination with `WALRecoveryMode::kTolerateCorruptedTailRecords`. Previously the two features were allowed together, which compromised the user's configured crash-recovery guarantees. * Fix a bug where a level refitting in CompactRange() might race with an automatic compaction that puts the data to the target level of the refitting. The bug has been there for years. +* BackupEngine::CreateNewBackup could fail intermittently with non-OK status when backing up a read-write DB configured with a DBOptions::file_checksum_gen_factory. This issue has been worked-around such that CreateNewBackup should succeed, but (until fully fixed) BackupEngine might not see all checksums available in the DB. ### New Features * A new option `std::shared_ptr file_checksum_gen_factory` is added to `BackupableDBOptions`. The default value for this option is `nullptr`. If this option is null, the default backup engine checksum function (crc32c) will be used for creating, verifying, or restoring backups. If it is not null and is set to the DB custom checksum factory, the custom checksum function used in DB will also be used for creating, verifying, or restoring backups, in addition to the default checksum function (crc32c). If it is not null and is set to a custom checksum factory different than the DB custom checksum factory (which may be null), BackupEngine will return `Status::InvalidArgument()`. diff --git a/utilities/backupable/backupable_db_test.cc b/utilities/backupable/backupable_db_test.cc index bec8a7ada..da5cb9f5e 100644 --- a/utilities/backupable/backupable_db_test.cc +++ b/utilities/backupable/backupable_db_test.cc @@ -1814,6 +1814,38 @@ TEST_F(BackupableDBTest, InterruptCreationTest) { AssertBackupConsistency(0, 0, keys_iteration); } +TEST_F(BackupableDBTest, FlushCompactDuringBackupCheckpoint) { + const int keys_iteration = 5000; + options_.file_checksum_gen_factory = GetFileChecksumGenCrc32cFactory(); + for (const auto& sopt : kAllShareOptions) { + OpenDBAndBackupEngine(true /* destroy_old_data */, false /* dummy */, sopt); + FillDB(db_.get(), 0, keys_iteration); + // That FillDB leaves a mix of flushed and unflushed data + SyncPoint::GetInstance()->LoadDependency( + {{"CheckpointImpl::CreateCustomCheckpoint:AfterGetLive1", + "BackupableDBTest::FlushDuringBackupCheckpoint:BeforeFlush"}, + {"BackupableDBTest::FlushDuringBackupCheckpoint:AfterFlush", + "CheckpointImpl::CreateCustomCheckpoint:AfterGetLive2"}}); + SyncPoint::GetInstance()->EnableProcessing(); + ROCKSDB_NAMESPACE::port::Thread flush_thread{[this]() { + TEST_SYNC_POINT( + "BackupableDBTest::FlushDuringBackupCheckpoint:BeforeFlush"); + FillDB(db_.get(), keys_iteration, 2 * keys_iteration); + ASSERT_OK(db_->Flush(FlushOptions())); + DBImpl* dbi = static_cast(db_.get()); + dbi->TEST_WaitForFlushMemTable(); + ASSERT_OK(db_->CompactRange(CompactRangeOptions(), nullptr, nullptr)); + dbi->TEST_WaitForCompact(); + TEST_SYNC_POINT( + "BackupableDBTest::FlushDuringBackupCheckpoint:AfterFlush"); + }}; + ASSERT_OK(backup_engine_->CreateNewBackup(db_.get())); + flush_thread.join(); + CloseDBAndBackupEngine(); + AssertBackupConsistency(0, 0, keys_iteration); + } +} + inline std::string OptionsPath(std::string ret, int backupID) { ret += "/private/"; ret += std::to_string(backupID); diff --git a/utilities/checkpoint/checkpoint_impl.cc b/utilities/checkpoint/checkpoint_impl.cc index 4fc6ede00..81ee48fdd 100644 --- a/utilities/checkpoint/checkpoint_impl.cc +++ b/utilities/checkpoint/checkpoint_impl.cc @@ -250,6 +250,8 @@ Status CheckpointImpl::CreateCustomCheckpoint( TEST_SYNC_POINT("CheckpointImpl::CreateCheckpoint:SavedLiveFiles2"); db_->FlushWAL(false /* sync */); } + TEST_SYNC_POINT("CheckpointImpl::CreateCustomCheckpoint:AfterGetLive1"); + TEST_SYNC_POINT("CheckpointImpl::CreateCustomCheckpoint:AfterGetLive2"); // if we have more than one column family, we need to also get WAL files if (s.ok()) { s = db_->GetSortedWalFiles(live_wal_files); @@ -314,8 +316,15 @@ Status CheckpointImpl::CreateCustomCheckpoint( // find checksum info for table files s = checksum_list->SearchOneFileChecksum(number, &checksum_value, &checksum_name); + + // XXX/FIXME(peterd): There's currently a race between GetLiveFiles + // and GetLiveFilesChecksumInfo that could lead to not finding + // checksum info on a file that has it. For now, we can accept + // that and treat it like a legacy file lacking checksum info. if (!s.ok()) { - return Status::NotFound("Can't find checksum for " + src_fname); + assert(checksum_name == kUnknownFileChecksumFuncName); + assert(checksum_value == kUnknownFileChecksum); + s = Status::OK(); } } s = copy_file_cb(db_->GetName(), src_fname,