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,