Removed check_flush_compaction_key_order (facebook#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: facebook#12311

Reviewed By: cbi42

Differential Revision: D53233379

Pulled By: ajkr

fbshipit-source-id: 1384361104021d6e3e580dce2ec123f9f99ce637

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

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_);
   }
 

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

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(

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<DBImpl>(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;

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) {

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

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

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

options/cf_options.cc

@@ -266,8 +266,7 @@ static std::unordered_map<std::string, OptionTypeInfo>
          {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",

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;

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

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;

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

unreleased_history/public_api_changes/remove_check_flush_compaction_key_order.md

@@ -0,0 +1 @@
+Removed deprecated option `ColumnFamilyOptions::check_flush_compaction_key_order`