Steps toward deprecating implicit prefix seek, related fixes #13026

pdillinger · 2024-09-20T04:26:26Z

Summary: With some new use cases onboarding to prefix extractors/seek/filters, one of the risks is existing iterator code, e.g. for maintenance tasks, being unintentionally subject to prefix seek semantics. This is a longstanding known design flaw with prefix seek, and prefix_same_as_start and auto_prefix_mode were steps in the direction of making that obsolete. However, we can't just immediately set total_order_seek to true by default, because that would impact so much code instantly.

Here we add a new DB option, prefix_seek_opt_in_only that basically allows users to transition to the future behavior when they are ready. When set to true, all iterators will be treated as if total_order_seek=true and then the only ways to get prefix seek semantics are with prefix_same_as_start or auto_prefix_mode.

Related fixes / changes:

Make sure that prefix_same_as_start and auto_prefix_mode are compatible with (or override) total_order_seek (depending on your interpretation).
Fix a bug in which a new iterator after dynamically changing the prefix extractor might mix different prefix semantics between memtable and SSTs. Both should use the latest extractor semantics, which means iterators ignoring memtable prefix filters with an old extractor. And that means passing the latest prefix extractor to new memtable iterators that might use prefix seek. (Without the fix, the test added for this fails in many ways.)

Suggested follow-up:

Investigate a FIXME where a MergeIteratorBuilder is created in db_impl.cc. No unit test detects a change in value that should impact correctness.
Make memtable prefix bloom compatible with auto_prefix_mode, which might require involving the memtablereps because we don't know at iterator creation time (only seek time) whether an auto_prefix_mode seek will be a prefix seek.
Add prefix_same_as_start testing to db_stress

Test Plan: tests updated, added. Add combination of total_order_seek=true and auto_prefix_mode=true to stress test. Ran make blackbox_crash_test for a long while.

Manually ran tests with prefix_seek_opt_in_only=true as default, looking for unexpected issues. I inspected most of the results and migrated many tests to be ready for such a change (but not all).

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

ltamasi · 2024-09-20T17:07:42Z

@pdillinger Heads up, the mini stress test failure might be legit (or the stress test framework might need some work)

pdillinger · 2024-09-20T17:23:37Z

@pdillinger Heads up, the mini stress test failure might be legit (or the stress test framework might need some work)

Yeah, looks like a bug or mismatched expectations. Investigating.

pdillinger · 2024-09-20T17:39:57Z

It looks like my memtable support for auto_prefix_mode is incomplete/insufficient, so I'll rip that back out.

ltamasi · 2024-09-20T17:21:49Z

db/column_family.cc

+      /*prefix_extractor=*/nullptr));
+  super_version->imm->AddIterators(
+      read_opts, /*seqno_to_time_mapping=*/nullptr,
+      super_version->mutable_cf_options.prefix_extractor.get(),


Q: Do we have to pass the prefix extractor here? We don't pass it to the memtable iterator, and MergeIteratorBuilder is in non-prefix mode

ltamasi · 2024-09-20T17:41:37Z

db/db_bloom_filter_test.cc

+    ropts.prefix_same_as_start = true;
+  }
+  std::unique_ptr<Iterator> iter(db_->NewIterator(ropts));
+  // Out of domain


Q: just to clarify, for out-of-domain queries, the expectation is that the prefix extractor (and any prefix blooms) will be ignored, right?

ltamasi · 2024-09-20T17:51:12Z

db/db_bloom_filter_test.cc

+  ASSERT_OK(Put("goat1", "g1"));
+  ASSERT_OK(Put("goat2", "g2"));
+
+  iter.reset(db_->NewIterator(ropts));


Maybe toss in some Gets too like above?

ltamasi · 2024-09-20T18:11:51Z

db/db_impl/db_impl.cc

+  read_options.total_order_seek |=
+      immutable_db_options_.prefix_seek_opt_in_only;
+


Is this the best (or only) place to do this? We also have NewIterators as well as some other features like transactions which call NewIteratorImpl directly

Fixing. I'll have an assertion in DBIter::DBIter that prefix_seek_opt_in_only has been applied to total_order_seek

ltamasi · 2024-09-20T18:26:51Z

db_stress_tool/db_stress_test_base.cc

@@ -1592,7 +1592,7 @@ Status StressTest::TestIterateImpl(ThreadState* thread,
    ro.total_order_seek = true;
    expect_total_order = true;
  } else if (thread->rand.OneIn(4)) {
-    ro.total_order_seek = false;
+    ro.total_order_seek = thread->rand.OneIn(2);


We would want to add the new option to the stress/crash test framework as well

It looks tricky to add because there's a surprising lot of stress test code doing prefix iteration. prefix_seek_opt_in_only is not unlocking any new functionality. It's just a cosmetic shortcut for forcing total_order_seek.

facebook-github-bot · 2024-09-20T21:14:47Z

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

pdillinger · 2024-09-20T21:21:57Z

This PR is not really showing the follow-up commits main...pdillinger:rocksdb:prefix_seek_opt_in_only

ltamasi

LGTM, thanks!

facebook-github-bot · 2024-09-20T23:25:14Z

@pdillinger merged this pull request in a1a102f.

facebook-github-bot added the CLA Signed label Sep 20, 2024

pdillinger force-pushed the prefix_seek_opt_in_only branch 2 times, most recently from 4ad0388 to a8d741e Compare September 20, 2024 15:39

pdillinger changed the title ~~WIP~~ Steps toward deprecating implicit prefix seek, related fixes Sep 20, 2024

WIP

cf2f5dd

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

pdillinger force-pushed the prefix_seek_opt_in_only branch from a8d741e to cf2f5dd Compare September 20, 2024 16:22

pdillinger marked this pull request as ready for review September 20, 2024 16:22

pdillinger requested a review from ltamasi September 20, 2024 16:38

Some clarification, release notes

7441351

pdillinger added 3 commits September 20, 2024 10:55

Revert attempted memtable auto_prefix_mode handling, other fixes

ae5cefe

make format

1204f5d

Specific test for prefix_seek_opt_in_only

597a9fc

ltamasi reviewed Sep 20, 2024

View reviewed changes

Different / more complete cut points for setting total_order_seek

38360b8

hx235 self-requested a review September 20, 2024 20:02

pdillinger added 2 commits September 20, 2024 13:55

Test updates

62bf4e2

Merge remote-tracking branch 'origin/main' into prefix_seek_opt_in_only

19a6a58

ltamasi approved these changes Sep 20, 2024

View reviewed changes

facebook-github-bot closed this in a1a102f Sep 20, 2024

facebook-github-bot added the Merged label Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Steps toward deprecating implicit prefix seek, related fixes #13026

Steps toward deprecating implicit prefix seek, related fixes #13026

pdillinger commented Sep 20, 2024 •

edited

Loading

ltamasi commented Sep 20, 2024

pdillinger commented Sep 20, 2024

pdillinger commented Sep 20, 2024

ltamasi Sep 20, 2024

pdillinger Sep 20, 2024

ltamasi Sep 20, 2024

pdillinger Sep 20, 2024

ltamasi Sep 20, 2024

pdillinger Sep 20, 2024

ltamasi Sep 20, 2024

pdillinger Sep 20, 2024

ltamasi Sep 20, 2024

pdillinger Sep 20, 2024

facebook-github-bot commented Sep 20, 2024

pdillinger commented Sep 20, 2024

ltamasi left a comment

facebook-github-bot commented Sep 20, 2024

		read_options.total_order_seek \|=
		immutable_db_options_.prefix_seek_opt_in_only;

Steps toward deprecating implicit prefix seek, related fixes #13026

Steps toward deprecating implicit prefix seek, related fixes #13026

Conversation

pdillinger commented Sep 20, 2024 • edited Loading

ltamasi commented Sep 20, 2024

pdillinger commented Sep 20, 2024

pdillinger commented Sep 20, 2024

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

Choose a reason for hiding this comment

facebook-github-bot commented Sep 20, 2024

pdillinger commented Sep 20, 2024

ltamasi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 20, 2024

pdillinger commented Sep 20, 2024 •

edited

Loading