[Draft] Add BlockBasedTableOptions::filter_construct_corruption to Options::DisableExtraCheck() #9479

hx235 · 2022-02-01T03:21:24Z

Include code in #9342 since it hasn't merged in. This is just a draft for discussion.

Context/Summary:
Turning on BlockBasedTableOptions::filter_construct_corruption can introduce extra cpu cost reflected in 20-30% increase in filter building time (See #9342 pr summary for more). Similar to force_consistency_checks in #9363, this PR brings BlockBasedTableOptions::filter_construct_corruption under Options::DisableExtraCheck().

…entries corruption return/todo for streaming hash entry verification/keep hash entries in AddAllEntries() by default and use list

Summary: -add // -abstact out BuiltinFilterBitsReader and static cast -specfic API comment related (new) bloom filter -initialize xor_checksum in-place -remove unnecesary large write buffer size in test -bracket initialization of uint64_t from literal -inject corruption in Finish path -more realistic subtle corruption in test

Summary: -Fix AlwaysTrueFilter length -Static GetBuiltinFilterBitsReader in BuiltinFilterPolicy -Fix API comment related to DisableExtraChecks Test Plan: Reviewers: Subscribers: Tasks: Tags:

hx235 · 2022-02-01T03:39:12Z

options/options.cc

+  BlockBasedTableOptions* table_options =
+      table_factory->GetOptions<BlockBasedTableOptions>();
+  table_options->detect_filter_construct_corruption = false;


Hey @pdillinger, I am learning more about how Options::DisableExtraChecks() is used. Is opening/reopening the db with the modified option from calling Options::DisableExtraChecks() a must in order for Options::DisableExtraChecks() to take effect? This somehow relates to my question below for Mark.

Hi @mrambacher :) I want to modify one particular field BlockBasedTableOptions::detect_filter_construct_corruption in the current option this but can't think of ways other than modifying through the raw pointer and reset the TableFactory like my impl above. This does not seem to be a standard practice to me based on the API doc and codebase search.

However, if we always open/reopen the db with the modified option containing the modified BlockBasedTableOptions (that is, YES to my question above to @pdillinger ), then it seems fine to modify through raw pointer based on the API doc(is it?).

If not, is there any other way of achieving my goal?

Found myself a potential alternative is to make detect_filter_construct_corruption mutable like #8620 and dynamically set it through SetOptions()

mrambacher · 2022-02-15T20:01:52Z

include/rocksdb/filter_policy.h

+  // WARNING: do not use a filter resulted from a corrupted construction
+  virtual Slice Finish(std::unique_ptr<const char[]>* buf,
+                       Status* /* status */) {
+    return Finish(buf);


Based on how the typical API signature, a signature returning a Status would be better:
Status Finish(std::unique_ptr<...>, Slice *slice)

I am reworking this signature in changes I am working on. (Also allowing a MemoryAllocator to be specified.)

@pdillinger Sounds good!

mrambacher · 2022-02-15T20:04:11Z

include/rocksdb/table.h

+  //
+  // This is an extra check that is only
+  // useful in detecting software bugs or CPU+memory malfunction.
+  // Turning on this feature increases filter construction time by 30%.


"... may substantially increase filter construction time"

Will fix in a separate tech debt PR

mrambacher · 2022-02-15T20:05:36Z

options/options_settable_test.cc

+      "filter_policy=bloomfilter:4:true;whole_key_filtering=1;detect_filter_"
+      "construct_corruption=false;"


Nit: This split in a funny way. Can you put the "detect_filter_construct_corrupion=false" on its own line please?

Will fix in a separate tech debt PR

mrambacher · 2022-02-15T20:05:49Z

options/options_test.cc

+      "filter_policy=bloomfilter:4.567:false;detect_filter_construct_"
+      "corruption=true;"


See above Nit

Will fix in a separate tech debt PR

mrambacher · 2022-02-15T20:07:07Z

table/block_based/block_based_table_builder.cc

+      assert(s.ok() || s.IsIncomplete() || s.IsCorruption());
+      if (s.IsCorruption()) {
+        rep_->SetStatus(s);
+        break;
+      }


Why not put the check for s.Corruption before the assert and leave the original?

Will fix in a separate tech debt PR

mrambacher · 2022-02-15T20:07:51Z

table/block_based/block_based_table_factory.cc

+          OptionType::kBoolean, OptionVerificationType::kNormal,
+          OptionTypeFlags::kNone}},


If you want to change it "on the fly" via SetOptions, it should be marked kMutable.

hx235 · 2022-02-15T20:21:07Z

Will adopt the comment and update the PR later! Thanks for the early review.

hx235 · 2022-04-05T16:06:56Z

will re-open when coming back to this

hx235 added 5 commits January 31, 2022 16:53

draft

614dff4

Experiemnt - partitioned_filters_construction_status_

495437b

Feeback: add to db_crasjtest.py/rename checksum/safer filter on hash …

49dfa1f

…entries corruption return/todo for streaming hash entry verification/keep hash entries in AddAllEntries() by default and use list

Feedback

3935564

Summary: -Fix AlwaysTrueFilter length -Static GetBuiltinFilterBitsReader in BuiltinFilterPolicy -Fix API comment related to DisableExtraChecks Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot added the CLA Signed label Feb 1, 2022

Add filter_construct_corruption to Options::DisableExtraCheck()

8899644

hx235 commented Feb 1, 2022

View reviewed changes

mrambacher reviewed Feb 15, 2022

View reviewed changes

hx235 closed this Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] Add BlockBasedTableOptions::filter_construct_corruption to Options::DisableExtraCheck() #9479

[Draft] Add BlockBasedTableOptions::filter_construct_corruption to Options::DisableExtraCheck() #9479

hx235 commented Feb 1, 2022 •

edited

Loading

hx235 Feb 1, 2022

hx235 Feb 1, 2022 •

edited

Loading

hx235 Feb 15, 2022

mrambacher Feb 15, 2022

pdillinger Feb 15, 2022

hx235 Feb 15, 2022

mrambacher Feb 15, 2022

hx235 Feb 15, 2022 •

edited

Loading

mrambacher Feb 15, 2022

hx235 Feb 15, 2022 •

edited

Loading

mrambacher Feb 15, 2022

hx235 Feb 15, 2022 •

edited

Loading

mrambacher Feb 15, 2022

hx235 Feb 15, 2022 •

edited

Loading

mrambacher Feb 15, 2022

hx235 commented Feb 15, 2022

hx235 commented Apr 5, 2022

		"filter_policy=bloomfilter:4:true;whole_key_filtering=1;detect_filter_"
		"construct_corruption=false;"

		"filter_policy=bloomfilter:4.567:false;detect_filter_construct_"
		"corruption=true;"

		OptionType::kBoolean, OptionVerificationType::kNormal,
		OptionTypeFlags::kNone}},

[Draft] Add BlockBasedTableOptions::filter_construct_corruption to Options::DisableExtraCheck() #9479

[Draft] Add BlockBasedTableOptions::filter_construct_corruption to Options::DisableExtraCheck() #9479

Conversation

hx235 commented Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

hx235 Feb 1, 2022 • edited Loading

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

hx235 Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hx235 Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hx235 Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hx235 Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hx235 commented Feb 15, 2022

hx235 commented Apr 5, 2022

hx235 commented Feb 1, 2022 •

edited

Loading

hx235 Feb 1, 2022 •

edited

Loading

hx235 Feb 15, 2022 •

edited

Loading

hx235 Feb 15, 2022 •

edited

Loading

hx235 Feb 15, 2022 •

edited

Loading

hx235 Feb 15, 2022 •

edited

Loading