From 75a1230ce85fb720e13252888599f6787b5ef449 Mon Sep 17 00:00:00 2001 From: Hui Xiao Date: Thu, 15 Aug 2024 12:32:59 -0700 Subject: [PATCH] Fix improper ExpectedValute::Exists() usages and disable compaction during VerifyDB() in crash test (#12933) Summary: **Context:** Adding assertion `!PendingPut()&&!PendingDelete()` in `ExpectedValute::Exists()` surfaced a couple improper usages of `ExpectedValute::Exists()` in the crash test - Commit phase of `ExpectedValue::Delete()`/`SyncDelete()`: When we issue delete to expected value during commit phase or `SyncDelete()` (used in crash recovery verification) as below, we don't really care about the result. https://github.com/facebook/rocksdb/blob/d458331ee90d0e8b5abd90e9f340d6857f7f679d/db_stress_tool/expected_state.cc#L73 https://github.com/facebook/rocksdb/blob/d458331ee90d0e8b5abd90e9f340d6857f7f679d/db_stress_tool/expected_value.cc#L52 That means, we don't really need to check for `Exists()` https://github.com/facebook/rocksdb/blob/d458331ee90d0e8b5abd90e9f340d6857f7f679d/db_stress_tool/expected_value.cc#L24-L26. This actually gives an alternative solution to https://github.com/facebook/rocksdb/commit/b65e29a4a905dfc94c20a6c6266cd8e9f8ea7448 to solve false-positive assertion violation. - TestMultiGetXX() path: `Exists()` is called without holding the lock as required https://github.com/facebook/rocksdb/blame/f63428bcc7c42308ec1a84e82787b8d5dbd322ae/db_stress_tool/no_batched_ops_stress.cc#L2688 ``` void MaybeAddKeyToTxnForRYW( ThreadState* thread, int column_family, int64_t key, Transaction* txn, std::unordered_map& ryw_expected_values) { assert(thread); assert(txn); SharedState* const shared = thread->shared; assert(shared); if (!shared->AllowsOverwrite(key) && shared->Exists(column_family, key)) { // Just do read your write checks for keys that allow overwrites. return; } // With a 1 in 10 probability, insert the just added key in the batch // into the transaction. This will create an overlap with the MultiGet // keys and exercise some corner cases in the code if (thread->rand.OneIn(10)) { ``` https://github.com/facebook/rocksdb/blob/f63428bcc7c42308ec1a84e82787b8d5dbd322ae/db_stress_tool/expected_state.h#L74-L76 The assertion also failed if db stress compaction filter was invoked before crash recovery verification (`VerifyDB()`->`VerifyOrSyncValue()`) finishes. https://github.com/facebook/rocksdb/blob/f63428bcc7c42308ec1a84e82787b8d5dbd322ae/db_stress_tool/db_stress_compaction_filter.h#L53 It failed because it can encounter a key with pending state when checking for `Exists()` since that key's expected state has not been sync-ed with db state in `VerifyOrSyncValue()`. https://github.com/facebook/rocksdb/blob/f63428bcc7c42308ec1a84e82787b8d5dbd322ae/db_stress_tool/no_batched_ops_stress.cc#L2579-L2591 **Summary:** This PR fixes above issues by - not checking `Exists()` in commit phase/`SyncDelete()` - using the concurrent version of key existence check like in other read - conditionally temporarily disabling compaction till after crash recovery verification succeeds() And add back the assertion `!PendingPut()&&!PendingDelete()` Pull Request resolved: https://github.com/facebook/rocksdb/pull/12933 Test Plan: Rehearsal CI Reviewed By: cbi42 Differential Revision: D61214889 Pulled By: hx235 fbshipit-source-id: ef25ba896e64330ddf330182314981516880c3e4 --- db_stress_tool/db_stress_common.h | 2 -- db_stress_tool/db_stress_compaction_filter.h | 2 +- db_stress_tool/db_stress_driver.cc | 5 ++++- db_stress_tool/db_stress_shared_state.h | 2 ++ db_stress_tool/db_stress_test_base.cc | 10 +++++++++- db_stress_tool/db_stress_test_base.h | 7 ++++++- db_stress_tool/expected_value.cc | 2 +- db_stress_tool/expected_value.h | 2 +- db_stress_tool/no_batched_ops_stress.cc | 8 +++++++- 9 files changed, 31 insertions(+), 9 deletions(-) diff --git a/db_stress_tool/db_stress_common.h b/db_stress_tool/db_stress_common.h index 5380fc2d58..fc21f769df 100644 --- a/db_stress_tool/db_stress_common.h +++ b/db_stress_tool/db_stress_common.h @@ -125,7 +125,6 @@ DECLARE_int32(level0_stop_writes_trigger); DECLARE_int32(block_size); DECLARE_int32(format_version); DECLARE_int32(index_block_restart_interval); -DECLARE_bool(disable_auto_compactions); DECLARE_int32(max_background_compactions); DECLARE_int32(num_bottom_pri_threads); DECLARE_int32(compaction_thread_pool_adjust_interval); @@ -318,7 +317,6 @@ DECLARE_int32(prepopulate_blob_cache); DECLARE_int32(approximate_size_one_in); DECLARE_bool(best_efforts_recovery); DECLARE_bool(skip_verifydb); -DECLARE_bool(enable_compaction_filter); DECLARE_bool(paranoid_file_checks); DECLARE_bool(fail_if_options_file_error); DECLARE_uint64(batch_protection_bytes_per_key); diff --git a/db_stress_tool/db_stress_compaction_filter.h b/db_stress_tool/db_stress_compaction_filter.h index 408bb48f3e..c67b9f2073 100644 --- a/db_stress_tool/db_stress_compaction_filter.h +++ b/db_stress_tool/db_stress_compaction_filter.h @@ -49,7 +49,7 @@ class DbStressCompactionFilter : public CompactionFilter { return Decision::kKeep; } // Reaching here means we acquired the lock. - + key_mutex->AssertHeld(); bool key_exists = state_->Exists(cf_id_, key_num); const bool allow_overwrite = state_->AllowsOverwrite(key_num); diff --git a/db_stress_tool/db_stress_driver.cc b/db_stress_tool/db_stress_driver.cc index 0da91d742a..d5fb3e6436 100644 --- a/db_stress_tool/db_stress_driver.cc +++ b/db_stress_tool/db_stress_driver.cc @@ -167,7 +167,10 @@ bool RunStressTestImpl(SharedState* shared) { {FileType::kWalFile}); } } - now = clock->NowMicros(); + if (ShouldDisableAutoCompactionsBeforeVerifyDb()) { + Status s = stress->EnableAutoCompaction(); + assert(s.ok()); + } fprintf(stdout, "%s Starting database operations\n", clock->TimeToString(now / 1000000).c_str()); diff --git a/db_stress_tool/db_stress_shared_state.h b/db_stress_tool/db_stress_shared_state.h index d1ae3c5978..cdd9f71708 100644 --- a/db_stress_tool/db_stress_shared_state.h +++ b/db_stress_tool/db_stress_shared_state.h @@ -45,6 +45,8 @@ DECLARE_int32(open_write_fault_one_in); DECLARE_int32(open_read_fault_one_in); DECLARE_int32(inject_error_severity); +DECLARE_bool(disable_auto_compactions); +DECLARE_bool(enable_compaction_filter); namespace ROCKSDB_NAMESPACE { class StressTest; diff --git a/db_stress_tool/db_stress_test_base.cc b/db_stress_tool/db_stress_test_base.cc index 922f0d6836..b63e5c87d3 100644 --- a/db_stress_tool/db_stress_test_base.cc +++ b/db_stress_tool/db_stress_test_base.cc @@ -3832,6 +3832,10 @@ void CheckAndSetOptionsForUserTimestamp(Options& options) { FLAGS_persist_user_defined_timestamps; } +bool ShouldDisableAutoCompactionsBeforeVerifyDb() { + return !FLAGS_disable_auto_compactions && FLAGS_enable_compaction_filter; +} + bool InitializeOptionsFromFile(Options& options) { DBOptions db_options; ConfigOptions config_options; @@ -3945,7 +3949,11 @@ void InitializeOptionsFromFlags( new WriteBufferManager(FLAGS_db_write_buffer_size, block_cache)); } options.memtable_whole_key_filtering = FLAGS_memtable_whole_key_filtering; - options.disable_auto_compactions = FLAGS_disable_auto_compactions; + if (ShouldDisableAutoCompactionsBeforeVerifyDb()) { + options.disable_auto_compactions = true; + } else { + options.disable_auto_compactions = FLAGS_disable_auto_compactions; + } options.max_background_compactions = FLAGS_max_background_compactions; options.max_background_flushes = FLAGS_max_background_flushes; options.compaction_style = diff --git a/db_stress_tool/db_stress_test_base.h b/db_stress_tool/db_stress_test_base.h index 372c761d40..cf6f174b8d 100644 --- a/db_stress_tool/db_stress_test_base.h +++ b/db_stress_tool/db_stress_test_base.h @@ -48,7 +48,11 @@ class StressTest { return FLAGS_sync_fault_injection || FLAGS_disable_wal || FLAGS_manual_wal_flush_one_in > 0; } - + Status EnableAutoCompaction() { + assert(options_.disable_auto_compactions); + Status s = db_->EnableAutoCompaction(column_families_); + return s; + } void CleanUp(); protected: @@ -447,5 +451,6 @@ void InitializeOptionsGeneral( // user-defined timestamp. void CheckAndSetOptionsForUserTimestamp(Options& options); +bool ShouldDisableAutoCompactionsBeforeVerifyDb(); } // namespace ROCKSDB_NAMESPACE #endif // GFLAGS diff --git a/db_stress_tool/expected_value.cc b/db_stress_tool/expected_value.cc index d280055a25..7cbcebabf2 100644 --- a/db_stress_tool/expected_value.cc +++ b/db_stress_tool/expected_value.cc @@ -21,7 +21,7 @@ void ExpectedValue::Put(bool pending) { } bool ExpectedValue::Delete(bool pending) { - if (!Exists()) { + if (pending && !Exists()) { return false; } if (pending) { diff --git a/db_stress_tool/expected_value.h b/db_stress_tool/expected_value.h index 7233efac16..428c389cb6 100644 --- a/db_stress_tool/expected_value.h +++ b/db_stress_tool/expected_value.h @@ -38,7 +38,7 @@ class ExpectedValue { : expected_value_(expected_value) {} bool Exists() const { - assert(!PendingWrite()); + assert(!PendingWrite() && !PendingDelete()); return !IsDeleted(); } diff --git a/db_stress_tool/no_batched_ops_stress.cc b/db_stress_tool/no_batched_ops_stress.cc index 8d71a0863b..1e628d7d2f 100644 --- a/db_stress_tool/no_batched_ops_stress.cc +++ b/db_stress_tool/no_batched_ops_stress.cc @@ -2587,6 +2587,8 @@ class NonBatchedOpsStressTest : public StressTest { // Value doesn't exist in db, update state to reflect that shared->SyncDelete(cf, key); return true; + } else { + assert(false); } } char expected_value_data[kValueMaxLen]; @@ -2685,7 +2687,11 @@ class NonBatchedOpsStressTest : public StressTest { SharedState* const shared = thread->shared; assert(shared); - if (!shared->AllowsOverwrite(key) && shared->Exists(column_family, key)) { + const ExpectedValue expected_value = + thread->shared->Get(column_family, key); + bool may_exist = !ExpectedValueHelper::MustHaveNotExisted(expected_value, + expected_value); + if (!shared->AllowsOverwrite(key) && may_exist) { // Just do read your write checks for keys that allow overwrites. return; }