From f9d45358ca3d5e90650cc4c4d55916a247fe66e5 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Wed, 31 Jan 2024 16:30:26 -0800 Subject: [PATCH] Removed `check_flush_compaction_key_order` (#12311) Summary: `check_flush_compaction_key_order` option was introduced for the key order checking online validation. It gave users the ability to disable the validation without downgrade in case the validation caused inefficiencies or false positives. Over time this validation has shown to be cheap and correct, so the option to disable it can now be removed. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12311 Reviewed By: cbi42 Differential Revision: D53233379 Pulled By: ajkr fbshipit-source-id: 1384361104021d6e3e580dce2ec123f9f99ce637 --- db/builder.cc | 8 ++--- db/compaction/compaction_job.cc | 3 -- db/compaction/compaction_outputs.h | 14 ++++----- db/compaction/compaction_service_job.cc | 4 +-- db/corruption_test.cc | 30 ------------------- db/db_test.cc | 7 ----- db/output_validator.cc | 20 +++++-------- db/output_validator.h | 5 +--- include/rocksdb/advanced_options.h | 10 ------- options/cf_options.cc | 5 +--- options/cf_options.h | 4 --- options/options.cc | 1 - options/options_helper.cc | 2 -- tools/db_bench_tool.cc | 7 ----- ...remove_check_flush_compaction_key_order.md | 1 + 15 files changed, 21 insertions(+), 100 deletions(-) create mode 100644 unreleased_history/public_api_changes/remove_check_flush_compaction_key_order.md diff --git a/db/builder.cc b/db/builder.cc index 54749b6893..d754d49559 100644 --- a/db/builder.cc +++ b/db/builder.cc @@ -83,11 +83,8 @@ Status BuildTable( auto& ioptions = tboptions.ioptions; // Reports the IOStats for flush for every following bytes. const size_t kReportFlushIOStatsEvery = 1048576; - OutputValidator output_validator( - tboptions.internal_comparator, - /*enable_order_check=*/ - mutable_cf_options.check_flush_compaction_key_order, - /*enable_hash=*/paranoid_file_checks); + OutputValidator output_validator(tboptions.internal_comparator, + /*enable_hash=*/paranoid_file_checks); Status s; meta->fd.file_size = 0; iter->SeekToFirst(); @@ -425,7 +422,6 @@ Status BuildTable( s = it->status(); if (s.ok() && paranoid_file_checks) { OutputValidator file_validator(tboptions.internal_comparator, - /*enable_order_check=*/true, /*enable_hash=*/true); for (it->SeekToFirst(); it->Valid(); it->Next()) { // Generate a rolling 64-bit hash of the key and values diff --git a/db/compaction/compaction_job.cc b/db/compaction/compaction_job.cc index 9dc4e31b9a..867f8090dc 100644 --- a/db/compaction/compaction_job.cc +++ b/db/compaction/compaction_job.cc @@ -760,7 +760,6 @@ Status CompactionJob::Run() { if (s.ok() && paranoid_file_checks_) { OutputValidator validator(cfd->internal_comparator(), - /*_enable_order_check=*/true, /*_enable_hash=*/true); for (iter->SeekToFirst(); iter->Valid(); iter->Next()) { s = validator.Add(iter->key(), iter->value()); @@ -1948,8 +1947,6 @@ Status CompactionJob::OpenCompactionOutputFile(SubcompactionState* sub_compact, } outputs.AddOutput(std::move(meta), cfd->internal_comparator(), - sub_compact->compaction->mutable_cf_options() - ->check_flush_compaction_key_order, paranoid_file_checks_); } diff --git a/db/compaction/compaction_outputs.h b/db/compaction/compaction_outputs.h index 18246cf2fa..f232214e3b 100644 --- a/db/compaction/compaction_outputs.h +++ b/db/compaction/compaction_outputs.h @@ -30,11 +30,9 @@ class CompactionOutputs { // compaction output file struct Output { Output(FileMetaData&& _meta, const InternalKeyComparator& _icmp, - bool _enable_order_check, bool _enable_hash, bool _finished, - uint64_t precalculated_hash) + bool _enable_hash, bool _finished, uint64_t precalculated_hash) : meta(std::move(_meta)), - validator(_icmp, _enable_order_check, _enable_hash, - precalculated_hash), + validator(_icmp, _enable_hash, precalculated_hash), finished(_finished) {} FileMetaData meta; OutputValidator validator; @@ -49,10 +47,10 @@ class CompactionOutputs { // Add generated output to the list void AddOutput(FileMetaData&& meta, const InternalKeyComparator& icmp, - bool enable_order_check, bool enable_hash, - bool finished = false, uint64_t precalculated_hash = 0) { - outputs_.emplace_back(std::move(meta), icmp, enable_order_check, - enable_hash, finished, precalculated_hash); + bool enable_hash, bool finished = false, + uint64_t precalculated_hash = 0) { + outputs_.emplace_back(std::move(meta), icmp, enable_hash, finished, + precalculated_hash); } // Set new table builder for the current output diff --git a/db/compaction/compaction_service_job.cc b/db/compaction/compaction_service_job.cc index 3149bb5002..442eaf8ea7 100644 --- a/db/compaction/compaction_service_job.cc +++ b/db/compaction/compaction_service_job.cc @@ -195,8 +195,8 @@ CompactionJob::ProcessKeyValueCompactionWithCompactionService( auto cfd = compaction->column_family_data(); sub_compact->Current().AddOutput(std::move(meta), - cfd->internal_comparator(), false, false, - true, file.paranoid_hash); + cfd->internal_comparator(), false, true, + file.paranoid_hash); } sub_compact->compaction_job_stats = compaction_result.stats; sub_compact->Current().SetNumOutputRecords( diff --git a/db/corruption_test.cc b/db/corruption_test.cc index 4e6a823687..e9e7344fd1 100644 --- a/db/corruption_test.cc +++ b/db/corruption_test.cc @@ -822,7 +822,6 @@ TEST_F(CorruptionTest, ParanoidFileChecksOnFlush) { Options options; options.level_compaction_dynamic_level_bytes = false; options.env = env_.get(); - options.check_flush_compaction_key_order = false; options.paranoid_file_checks = true; options.create_if_missing = true; Status s; @@ -853,7 +852,6 @@ TEST_F(CorruptionTest, ParanoidFileChecksOnCompact) { options.env = env_.get(); options.paranoid_file_checks = true; options.create_if_missing = true; - options.check_flush_compaction_key_order = false; Status s; for (const auto& mode : corruption_modes) { delete db_; @@ -885,7 +883,6 @@ TEST_F(CorruptionTest, ParanoidFileChecksWithDeleteRangeFirst) { Options options; options.level_compaction_dynamic_level_bytes = false; options.env = env_.get(); - options.check_flush_compaction_key_order = false; options.paranoid_file_checks = true; options.create_if_missing = true; for (bool do_flush : {true, false}) { @@ -922,7 +919,6 @@ TEST_F(CorruptionTest, ParanoidFileChecksWithDeleteRange) { Options options; options.level_compaction_dynamic_level_bytes = false; options.env = env_.get(); - options.check_flush_compaction_key_order = false; options.paranoid_file_checks = true; options.create_if_missing = true; for (bool do_flush : {true, false}) { @@ -962,7 +958,6 @@ TEST_F(CorruptionTest, ParanoidFileChecksWithDeleteRangeLast) { Options options; options.level_compaction_dynamic_level_bytes = false; options.env = env_.get(); - options.check_flush_compaction_key_order = false; options.paranoid_file_checks = true; options.create_if_missing = true; for (bool do_flush : {true, false}) { @@ -1031,7 +1026,6 @@ TEST_F(CorruptionTest, CompactionKeyOrderCheck) { options.env = env_.get(); options.paranoid_file_checks = false; options.create_if_missing = true; - options.check_flush_compaction_key_order = false; delete db_; db_ = nullptr; ASSERT_OK(DestroyDB(dbname_, options)); @@ -1046,7 +1040,6 @@ TEST_F(CorruptionTest, CompactionKeyOrderCheck) { ASSERT_OK(dbi->TEST_FlushMemTable()); mock->SetCorruptionMode(mock::MockTableFactory::kCorruptNone); - ASSERT_OK(db_->SetOptions({{"check_flush_compaction_key_order", "true"}})); CompactRangeOptions cro; cro.bottommost_level_compaction = BottommostLevelCompaction::kForce; ASSERT_NOK( @@ -1059,7 +1052,6 @@ TEST_F(CorruptionTest, FlushKeyOrderCheck) { options.env = env_.get(); options.paranoid_file_checks = false; options.create_if_missing = true; - ASSERT_OK(db_->SetOptions({{"check_flush_compaction_key_order", "true"}})); ASSERT_OK(db_->Put(WriteOptions(), "foo1", "v1")); ASSERT_OK(db_->Put(WriteOptions(), "foo2", "v1")); @@ -1084,28 +1076,6 @@ TEST_F(CorruptionTest, FlushKeyOrderCheck) { ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->ClearAllCallBacks(); } -TEST_F(CorruptionTest, DisableKeyOrderCheck) { - ASSERT_OK(db_->SetOptions({{"check_flush_compaction_key_order", "false"}})); - DBImpl* dbi = static_cast_with_check(db_); - - SyncPoint::GetInstance()->SetCallBack( - "OutputValidator::Add:order_check", - [&](void* /*arg*/) { ASSERT_TRUE(false); }); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing(); - ASSERT_OK(db_->Put(WriteOptions(), "foo1", "v1")); - ASSERT_OK(db_->Put(WriteOptions(), "foo3", "v1")); - ASSERT_OK(dbi->TEST_FlushMemTable()); - ASSERT_OK(db_->Put(WriteOptions(), "foo2", "v1")); - ASSERT_OK(db_->Put(WriteOptions(), "foo4", "v1")); - ASSERT_OK(dbi->TEST_FlushMemTable()); - CompactRangeOptions cro; - cro.bottommost_level_compaction = BottommostLevelCompaction::kForce; - ASSERT_OK( - dbi->CompactRange(cro, dbi->DefaultColumnFamily(), nullptr, nullptr)); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing(); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->ClearAllCallBacks(); -} - TEST_F(CorruptionTest, VerifyWholeTableChecksum) { CloseDb(); Options options; diff --git a/db/db_test.cc b/db/db_test.cc index 4acc744f08..b47d706fe5 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -5813,13 +5813,6 @@ TEST_F(DBTest, DynamicMiscOptions) { ASSERT_OK(dbfull()->TEST_GetLatestMutableCFOptions(handles_[1], &mutable_cf_options)); ASSERT_TRUE(mutable_cf_options.report_bg_io_stats); - ASSERT_TRUE(mutable_cf_options.check_flush_compaction_key_order); - - ASSERT_OK(dbfull()->SetOptions( - handles_[1], {{"check_flush_compaction_key_order", "false"}})); - ASSERT_OK(dbfull()->TEST_GetLatestMutableCFOptions(handles_[1], - &mutable_cf_options)); - ASSERT_FALSE(mutable_cf_options.check_flush_compaction_key_order); } TEST_F(DBTest, L0L1L2AndUpHitCounter) { diff --git a/db/output_validator.cc b/db/output_validator.cc index e93e2d68c4..0c7109f3c2 100644 --- a/db/output_validator.cc +++ b/db/output_validator.cc @@ -15,19 +15,15 @@ Status OutputValidator::Add(const Slice& key, const Slice& value) { paranoid_hash_ = NPHash64(key.data(), key.size(), paranoid_hash_); paranoid_hash_ = NPHash64(value.data(), value.size(), paranoid_hash_); } - if (enable_order_check_) { - TEST_SYNC_POINT_CALLBACK("OutputValidator::Add:order_check", - /*arg=*/nullptr); - if (key.size() < kNumInternalBytes) { - return Status::Corruption( - "Compaction tries to write a key without internal bytes."); - } - // prev_key_ starts with empty. - if (!prev_key_.empty() && icmp_.Compare(key, prev_key_) < 0) { - return Status::Corruption("Compaction sees out-of-order keys."); - } - prev_key_.assign(key.data(), key.size()); + if (key.size() < kNumInternalBytes) { + return Status::Corruption( + "Compaction tries to write a key without internal bytes."); } + // prev_key_ starts with empty. + if (!prev_key_.empty() && icmp_.Compare(key, prev_key_) < 0) { + return Status::Corruption("Compaction sees out-of-order keys."); + } + prev_key_.assign(key.data(), key.size()); return Status::OK(); } } // namespace ROCKSDB_NAMESPACE diff --git a/db/output_validator.h b/db/output_validator.h index 40635f9c44..1e7a8988ed 100644 --- a/db/output_validator.h +++ b/db/output_validator.h @@ -15,12 +15,10 @@ namespace ROCKSDB_NAMESPACE { // of all the key and value. class OutputValidator { public: - explicit OutputValidator(const InternalKeyComparator& icmp, - bool enable_order_check, bool enable_hash, + explicit OutputValidator(const InternalKeyComparator& icmp, bool enable_hash, uint64_t precalculated_hash = 0) : icmp_(icmp), paranoid_hash_(precalculated_hash), - enable_order_check_(enable_order_check), enable_hash_(enable_hash) {} // Add a key to the KV sequence, and return whether the key follows @@ -42,7 +40,6 @@ class OutputValidator { const InternalKeyComparator& icmp_; std::string prev_key_; uint64_t paranoid_hash_ = 0; - bool enable_order_check_; bool enable_hash_; }; } // namespace ROCKSDB_NAMESPACE diff --git a/include/rocksdb/advanced_options.h b/include/rocksdb/advanced_options.h index fd51e22657..a5c2eba7ef 100644 --- a/include/rocksdb/advanced_options.h +++ b/include/rocksdb/advanced_options.h @@ -701,16 +701,6 @@ struct AdvancedColumnFamilyOptions { // Default: false bool optimize_filters_for_hits = false; - // DEPRECATED: This option might be removed in a future release. - // - // During flush or compaction, check whether keys inserted to output files - // are in order. - // - // Default: true - // - // Dynamically changeable through SetOptions() API - bool check_flush_compaction_key_order = true; - // After writing every SST file, reopen it and read all the keys. // Checks the hash of all of the keys and values written versus the // keys in the file and signals a corruption if they do not match diff --git a/options/cf_options.cc b/options/cf_options.cc index cad6e2ac09..e4b5a7d3a2 100644 --- a/options/cf_options.cc +++ b/options/cf_options.cc @@ -266,8 +266,7 @@ static std::unordered_map {0, OptionType::kBoolean, OptionVerificationType::kDeprecated, OptionTypeFlags::kMutable}}, {"check_flush_compaction_key_order", - {offsetof(struct MutableCFOptions, check_flush_compaction_key_order), - OptionType::kBoolean, OptionVerificationType::kNormal, + {0, OptionType::kBoolean, OptionVerificationType::kDeprecated, OptionTypeFlags::kMutable}}, {"paranoid_file_checks", {offsetof(struct MutableCFOptions, paranoid_file_checks), @@ -1111,8 +1110,6 @@ void MutableCFOptions::Dump(Logger* log) const { result.c_str()); ROCKS_LOG_INFO(log, " max_sequential_skip_in_iterations: %" PRIu64, max_sequential_skip_in_iterations); - ROCKS_LOG_INFO(log, " check_flush_compaction_key_order: %d", - check_flush_compaction_key_order); ROCKS_LOG_INFO(log, " paranoid_file_checks: %d", paranoid_file_checks); ROCKS_LOG_INFO(log, " report_bg_io_stats: %d", diff --git a/options/cf_options.h b/options/cf_options.h index f42d6b5629..f6dea50316 100644 --- a/options/cf_options.h +++ b/options/cf_options.h @@ -160,8 +160,6 @@ struct MutableCFOptions { prepopulate_blob_cache(options.prepopulate_blob_cache), max_sequential_skip_in_iterations( options.max_sequential_skip_in_iterations), - check_flush_compaction_key_order( - options.check_flush_compaction_key_order), paranoid_file_checks(options.paranoid_file_checks), report_bg_io_stats(options.report_bg_io_stats), compression(options.compression), @@ -221,7 +219,6 @@ struct MutableCFOptions { blob_file_starting_level(0), prepopulate_blob_cache(PrepopulateBlobCache::kDisable), max_sequential_skip_in_iterations(0), - check_flush_compaction_key_order(true), paranoid_file_checks(false), report_bg_io_stats(false), compression(Snappy_Supported() ? kSnappyCompression : kNoCompression), @@ -311,7 +308,6 @@ struct MutableCFOptions { // Misc options uint64_t max_sequential_skip_in_iterations; - bool check_flush_compaction_key_order; bool paranoid_file_checks; bool report_bg_io_stats; CompressionType compression; diff --git a/options/options.cc b/options/options.cc index 2e7c025030..9690862b44 100644 --- a/options/options.cc +++ b/options/options.cc @@ -533,7 +533,6 @@ Options* Options::DisableExtraChecks() { // See https://github.com/facebook/rocksdb/issues/9354 force_consistency_checks = false; // Considered but no clear performance impact seen: - // * check_flush_compaction_key_order // * paranoid_checks // * flush_verify_memtable_count // By current API contract, not including diff --git a/options/options_helper.cc b/options/options_helper.cc index fa5d549c18..99895680f5 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -266,8 +266,6 @@ void UpdateColumnFamilyOptions(const MutableCFOptions& moptions, // Misc options cf_opts->max_sequential_skip_in_iterations = moptions.max_sequential_skip_in_iterations; - cf_opts->check_flush_compaction_key_order = - moptions.check_flush_compaction_key_order; cf_opts->paranoid_file_checks = moptions.paranoid_file_checks; cf_opts->report_bg_io_stats = moptions.report_bg_io_stats; cf_opts->compression = moptions.compression; diff --git a/tools/db_bench_tool.cc b/tools/db_bench_tool.cc index 3bfbf685b3..29fe3f8e1f 100644 --- a/tools/db_bench_tool.cc +++ b/tools/db_bench_tool.cc @@ -930,11 +930,6 @@ DEFINE_bool(force_consistency_checks, "Runs consistency checks on the LSM every time a change is " "applied."); -DEFINE_bool(check_flush_compaction_key_order, - ROCKSDB_NAMESPACE::Options().check_flush_compaction_key_order, - "During flush or compaction, check whether keys inserted to " - "output files are in order."); - DEFINE_uint64(delete_obsolete_files_period_micros, 0, "Ignored. Left here for backward compatibility"); @@ -4617,8 +4612,6 @@ class Benchmark { options.optimize_filters_for_hits = FLAGS_optimize_filters_for_hits; options.paranoid_checks = FLAGS_paranoid_checks; options.force_consistency_checks = FLAGS_force_consistency_checks; - options.check_flush_compaction_key_order = - FLAGS_check_flush_compaction_key_order; options.periodic_compaction_seconds = FLAGS_periodic_compaction_seconds; options.ttl = FLAGS_ttl_seconds; // fill storage options diff --git a/unreleased_history/public_api_changes/remove_check_flush_compaction_key_order.md b/unreleased_history/public_api_changes/remove_check_flush_compaction_key_order.md new file mode 100644 index 0000000000..d06559bf13 --- /dev/null +++ b/unreleased_history/public_api_changes/remove_check_flush_compaction_key_order.md @@ -0,0 +1 @@ +Removed deprecated option `ColumnFamilyOptions::check_flush_compaction_key_order`