Introduce options.check_flush_compaction_key_order #7467

siying · 2020-09-30T04:59:24Z

Summary:
Introduce an new option options.check_flush_compaction_key_order, by default set to true, which checks key order of flush and compaction, and fail the operation if the order is violated.
Also did minor refactor hash checking code, which consolidates the hashing logic to a vlidation class, where the key ordering logic is added.

Test Plan: Add unit tests to validate the check can catch reordering in flush and compaction, and can be properly disabled.

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-09-30T05:22:16Z

@siying has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

riversand963

Thanks @siying for the PR. LGTM with a few minor comments.

riversand963 · 2020-09-30T05:29:32Z

db/output_validator.h

+      paranoid_hash_ =
+          Hash64(value.data(), value.size(), paranoid_hash_);
+    }
+    if (enable_order_check_) {


I feel that if we enable hash check, then we should also enable order check since its overhead is relatively low.

The order check is likely to be more expensive than hash check. It needs to make a copy to every key, which is not cheap.

riversand963 · 2020-09-30T05:32:42Z

db/compaction/compaction_job.cc

+    meta.file_creation_time = current_time;
+    sub_compact->outputs.emplace_back(
+        std::move(meta), cfd->internal_comparator(),
+        /*enable_order_check*/


/*enable_order_check=*/

riversand963 · 2020-09-30T05:33:04Z

db/builder.cc

-  uint64_t paranoid_hash = 0;
+  OutputValidator output_validator(
+      internal_comparator,
+      /*enable_order_check*/


/*enable_order_check=*/

riversand963 · 2020-09-30T05:38:23Z

db/corruption_test.cc

+  ASSERT_OK(db_->SetOptions({{"check_flush_compaction_key_order", "false"}}));
+  DBImpl* dbi = static_cast_with_check<DBImpl>(db_);
+
+  SyncPoint::GetInstance()->SetCallBack(


Nit: maybe clear all callbacks after the test.

riversand963 · 2020-09-30T05:40:14Z

db/output_validator.h

+  icmp_(_icmp), enable_order_check_(_enable_order_check), enable_hash_(_enable_hash) {}
+  Status Add(const Slice& key, const Slice& value) {
+    if (enable_hash_) {
+      // Generate a rolling 64-bit hash of the key and values


Just curious, any plan to make the hash algorithm configurable?

The hash is not exposed to users, unless the hash is not good enough I don't see a reason to make it configerable.

riversand963 · 2020-09-30T06:10:02Z

hmm, the test failures seem legit and need be fixed.

mrambacher · 2020-09-30T10:44:54Z

db/builder.cc

        }
        s = it->status();
-        if (s.ok() && check_hash != paranoid_hash) {
+        if (s.ok() && output_validator.GetHash() != file_validator.GetHash()) {


It might be better to move this check inside the OutputValidator, so that others can be added later.

mrambacher · 2020-09-30T10:50:49Z

db/output_validator.h

+    }
+    if (enable_order_check_) {
+      TEST_SYNC_POINT_CALLBACK("OutputValidator::Add:order_check", /*arg=*/nullptr);
+      if (key.size() < 8) {


Any chance of making "8" a constant? Much easier to find and understand...

mrambacher · 2020-09-30T10:52:06Z

db/output_validator.h

+                           bool _enable_order_check, bool _enable_hash) :
+  icmp_(_icmp), enable_order_check_(_enable_order_check), enable_hash_(_enable_hash) {}


I thought the coding standard only used an underscore prefix when assigning to structs, not classes?

mrambacher · 2020-09-30T11:00:00Z

db/output_validator.h

+  Status Add(const Slice& key, const Slice& value) {
+    if (enable_hash_) {


Can we move the implementation into a C++ file instead of a header and clean up the includes?

mrambacher · 2020-09-30T11:01:19Z

db/memtable.cc

@@ -375,6 +375,7 @@ class MemTableIterator : public InternalIterator {
    PERF_COUNTER_ADD(next_on_memtable_count, 1);
    assert(Valid());
    iter_->Next();
+    TEST_SYNC_POINT_CALLBACK("MemTableIterator::Next:0", iter_);


Just curious as to what this has to do with this change?

I didn't find a good way to fake a memtable that generates out of order keys. If there is a better way, will do it.

mrambacher · 2020-09-30T11:03:16Z

db/corruption_test.cc

+  Status s;
+  delete db_;
+  db_ = nullptr;
+  s = DestroyDB(dbname_, options);


Should this status be asserted?

mrambacher · 2020-09-30T11:05:26Z

db/compaction/compaction_job.cc

+      sub_compact->AddToBuilder(kv.first.Encode(), kv.second)
+          .PermitUncheckedError();


Don't you need to check this Status to see out of order keys?

facebook-github-bot · 2020-09-30T17:47:20Z

@siying has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-09-30T18:22:00Z

@siying has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-09-30T18:36:41Z

@siying has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-09-30T19:16:00Z

@siying has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ajkr

Awesome!

ajkr · 2020-09-30T21:43:29Z

db/compaction/compaction_job.cc

-      sub_compact->AddToBuilder(kv.first.Encode(), kv.second,
-                                paranoid_file_checks_);
+      // Range tombstone is not supported by output validator yet.
+      sub_compact->builder->Add(kv.first.Encode(), kv.second);


This fixes the problem with paranoid_file_checks + DeleteRange(), right? @mrambacher would it solve the problem to extract this into a separate PR that we can easily backport to 6.12 and 6.13?

Summary: Introduce an new option options.check_flush_compaction_key_order, by default set to true, which checks key order of flush and compaction, and fail the operation if the order is violated. Also did minor refactor hash checking code, which consolidates the hashing logic to a vlidation class, where the key ordering logic is added. Test Plan: Add unit tests to validate the check can catch reordering in flush and compaction, and can be properly disabled.

facebook-github-bot · 2020-10-01T02:57:39Z

@siying has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-10-01T03:00:01Z

@siying has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-01T18:40:24Z

This pull request has been merged in 7508175.

Summary: Introduce an new option options.check_flush_compaction_key_order, by default set to true, which checks key order of flush and compaction, and fail the operation if the order is violated. Also did minor refactor hash checking code, which consolidates the hashing logic to a vlidation class, where the key ordering logic is added. Pull Request resolved: facebook#7467 Test Plan: Add unit tests to validate the check can catch reordering in flush and compaction, and can be properly disabled. Reviewed By: riversand963 Differential Revision: D24010683 fbshipit-source-id: 8dd6292d2cda8006054e9ded7cfa4bf405f0527c

siying requested review from mrambacher and ajkr September 30, 2020 04:59

facebook-github-bot added the CLA Signed label Sep 30, 2020

facebook-github-bot reviewed Sep 30, 2020

View reviewed changes

riversand963 approved these changes Sep 30, 2020

View reviewed changes

mrambacher reviewed Sep 30, 2020

View reviewed changes

facebook-github-bot reviewed Sep 30, 2020

View reviewed changes

siying force-pushed the validate_compact branch from 4927da0 to b5bbce3 Compare September 30, 2020 18:21

facebook-github-bot reviewed Sep 30, 2020

View reviewed changes

siying force-pushed the validate_compact branch from b5bbce3 to 3a68b96 Compare September 30, 2020 18:36

facebook-github-bot reviewed Sep 30, 2020

View reviewed changes

ajkr approved these changes Sep 30, 2020

View reviewed changes

siying added 7 commits September 30, 2020 19:50

Fix builds of two tests.

e3676c3

Fix some tests.

ea78b7c

Address comments and fix two tests.

5a8cd82

Avoid this check for range tombstones.

b2d4411

Fix namespace

c455dbf

Bucketify

bc3a65c

siying force-pushed the validate_compact branch from 48556c0 to 3801434 Compare October 1, 2020 02:57

Supress a linter error.

2a80b4b

siying force-pushed the validate_compact branch from 3801434 to 2a80b4b Compare October 1, 2020 02:59

facebook-github-bot reviewed Oct 1, 2020

View reviewed changes

facebook-github-bot closed this in 7508175 Oct 1, 2020

facebook-github-bot added the Merged label Oct 1, 2020

ajkr mentioned this pull request Oct 4, 2020

sanity check key ordering during flush/compaction #7408

Closed

ajkr mentioned this pull request Oct 26, 2020

use OutputValidator in SstFileWriter #7599

Open

		bool _enable_order_check, bool _enable_hash) :
		icmp_(_icmp), enable_order_check_(_enable_order_check), enable_hash_(_enable_hash) {}

		Status Add(const Slice& key, const Slice& value) {
		if (enable_hash_) {

		sub_compact->AddToBuilder(kv.first.Encode(), kv.second)
		.PermitUncheckedError();

Introduce options.check_flush_compaction_key_order #7467

Introduce options.check_flush_compaction_key_order #7467

Conversation

siying commented Sep 30, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 30, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riversand963 commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 30, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 30, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 30, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 30, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 1, 2020

facebook-github-bot commented Oct 1, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 1, 2020