diff --git a/db/db_filesnapshot.cc b/db/db_filesnapshot.cc index b2eacb810e..4eb0446215 100644 --- a/db/db_filesnapshot.cc +++ b/db/db_filesnapshot.cc @@ -221,6 +221,12 @@ Status DBImpl::CreateOptionsFileForSubsetCheckpoint( return Status::OK(); } + // The snapshot's CF options are shallow copies that share live pointer + // sub-options (e.g. RibbonFilterPolicy, whose bloom_before_level is mutated + // in place by SetOptions). Hold options_mutex_, as WriteOptionsFile does, so + // SetOptions cannot change them between writing the file and verifying it. + InstrumentedMutexLock ol(&options_mutex_); + const std::string options_path = OptionsFileName(checkpoint_dir, options_snapshot.options_file_number); return PersistRocksDBOptions( diff --git a/utilities/checkpoint/checkpoint_test.cc b/utilities/checkpoint/checkpoint_test.cc index b9f6bbf158..9711aecb3a 100644 --- a/utilities/checkpoint/checkpoint_test.cc +++ b/utilities/checkpoint/checkpoint_test.cc @@ -14,6 +14,7 @@ #include #endif #include +#include #include #include #include @@ -31,10 +32,12 @@ #include "rocksdb/db.h" #include "rocksdb/env.h" #include "rocksdb/file_system.h" +#include "rocksdb/filter_policy.h" #include "rocksdb/metadata.h" #include "rocksdb/rocksdb_namespace.h" #include "rocksdb/sst_file_manager.h" #include "rocksdb/statistics.h" +#include "rocksdb/table.h" #include "rocksdb/utilities/backup_engine.h" #include "rocksdb/utilities/options_util.h" #include "rocksdb/utilities/transaction_db.h" @@ -780,6 +783,60 @@ TEST_F(CheckpointTest, SubsetOptionsAndManifestUseSameMembershipSnapshot) { VerifyDefaultAndTwoSubsetCheckpoint(env_, snapshot_name_, options); } +TEST_F(CheckpointTest, SubsetOptionsFileRacesWithMutableFilterPolicy) { + // RibbonFilterPolicy::bloom_before_level is mutated in place by SetOptions, + // and the subset checkpoint's options snapshot shares the live policy + // object. Persisting that snapshot must not interleave with SetOptions. + Options options = CurrentOptions(); + BlockBasedTableOptions table_options; + table_options.filter_policy.reset( + NewRibbonFilterPolicy(10, /*bloom_before_level=*/1)); + options.table_factory.reset(NewBlockBasedTableFactory(table_options)); + CreateAndReopenWithCF({"one", "two", "three"}, options); + PopulateFourCfsForSubsetCheckpoint(db_.get(), handles_); + + const std::thread::id checkpoint_thread_id = std::this_thread::get_id(); + std::atomic injected{false}; + std::atomic set_options_done{false}; + Status set_options_status; + std::thread set_options_thread; + SyncPoint::GetInstance()->SetCallBack( + "PersistRocksDBOptions:written", [&](void*) { + if (std::this_thread::get_id() != checkpoint_thread_id || + injected.exchange(true)) { + return; + } + set_options_thread = std::thread([&]() { + set_options_status = db_->SetOptions( + handles_[0], + {{"table_factory.filter_policy.bloom_before_level", "2"}}); + set_options_done.store(true); + }); + // Give an unserialized SetOptions the chance to land between writing + // the checkpoint OPTIONS file and verifying it. + for (int i = 0; i < 100 && !set_options_done.load(); ++i) { + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + }); + SyncPoint::GetInstance()->EnableProcessing(); + Defer sync_point_cleanup([]() { + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + }); + + Checkpoint* checkpoint; + ASSERT_OK(Checkpoint::Create(db_.get(), &checkpoint)); + std::unique_ptr uchk(checkpoint); + Status checkpoint_status = + uchk->CreateCheckpoint(snapshot_name_, {handles_[0], handles_[2]}); + if (set_options_thread.joinable()) { + set_options_thread.join(); + } + ASSERT_TRUE(injected.load()); + ASSERT_OK(set_options_status); + ASSERT_OK(checkpoint_status); +} + TEST_F(CheckpointTest, CheckpointSubsetIncompleteAtomicGroupAfterWritableRecovery) { Options options = CurrentOptions();