Fix a seek issue with prefix extractor and timestamp #7644

jay-zhuang · 2020-11-05T23:55:50Z

During seek, prefix compare should not include timestamp.

Test Plan: added unittest

facebook-github-bot

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

riversand963

Thanks @jay-zhuang for the fix. Mostly LGTM except a comment on test.

riversand963 · 2020-11-06T00:15:09Z

db/db_with_timestamp_basic_test.cc

+  Options options = CurrentOptions();
+  options.env = env_;
+  options.create_if_missing = true;
+  options.prefix_extractor.reset(NewFixedPrefixTransform(3));


Can we use a prefix extractor that does not simply extract a prefix from user key?
If the prefix extractor takes <user_key, ts> and just returns a substring of user_key, then the fix introduced in this PR won't actually make a difference.
Is there a prefix extractor that takes <user_key, ts> and returns a result based on both user_key and ts?

I never thought about such usecase. Are you saying there could be usecase like: get prefix with the whole key + part of timestamp?
For example, with the following data:

foo1, ts1 -> value1 foo1, ts2 -> value2 foo1, ts3 -> value3

And prefix extractor is: key + first_part_of(ts). Is that correct? Or a valid case? And I assume the prefix extractor can only return a prefix of the user key, so to include timestamp, it has to include the whole key. Is that right?

This unittest just to reproduce the issue that the prefix key is compared with CompareWithoutTimestamp() instead of Compare().

I believe so, according to a discussion with @pdillinger. #7589 (comment)

Some of our existing code feeds <user_key, ts> as input to prefix extractor, and we cannot assume that the ts part does not affect output of prefix extraction.

I don't think we should include timestamp for prefix extractor:

it's an internal implementation detail that we should not expose to the user. That we have timestamp right after user key and how the timestamp is encoded (I think you mentioned that we might remove timestamp if the data is on the bottom level like seq_num and maybe delta encoding?);

All user's existing prefix extractors need to update to handle the timestamp, as the prefix may or may not include the timestamp:

rocksdb/util/slice.cc

Line 26 in fdf882d

explicit FixedPrefixTransform(size_t prefix_len)

I don't feel it's a useful usecase (if it's still valid) to include timestamp in prefix extractor. If we do need it, we should have a new prefix extractor interface to explicitly having the timestamp.

What you think?

Sorry for the confusion, but I don't think the unit test is actually testing what we meant to fix. In fact, if just change version_set.cc as follows, the unit test still can pass.

diff --git a/db/version_set.cc b/db/version_set.cc index 0fe09ef34..1d4ef1853 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1100,9 +1100,9 @@ void LevelIterator::Seek(const Slice& target) { Slice file_user_key = ExtractUserKey(file_iter_.key()); if (prefix_extractor_->InDomain(target_user_key) && (!prefix_extractor_->InDomain(file_user_key) || - user_comparator_.Compare( - prefix_extractor_->Transform(target_user_key), - prefix_extractor_->Transform(file_user_key)) != 0)) { + user_comparator_.CompareWithoutTimestamp( + prefix_extractor_->Transform(target_user_key), false, + prefix_extractor_->Transform(file_user_key), false) != 0)) { SetFileIterator(nullptr); } }

Namely, without stripping the timestamp from user key, the unit test can still pass.
I was saying we need a test case in which your change will make the unit pass, but the above will not.

My original comment was proposing a test case which, without your change, will mistakenly include timestamp for prefix extraction and yield a different result.

That makes sense. Added one.

rocksdb/include/rocksdb/options.h

Lines 225 to 232 in c3911f1

// db.NewIterator(). For prefix filtering to work properly,

// "prefix_extractor" and "comparator" must be such that the following

// properties hold:

//

// 1) key.starts_with(prefix(key))

// 2) Compare(prefix(key), key) <= 0.

// 3) If Compare(k1, k2) <= 0, then Compare(prefix(k1), prefix(k2)) <= 0

// 4) prefix(prefix(key)) == prefix(key)

I think we have to assume things might break if "prefix" is not actually a prefix of user key (without timestamp), but I haven't thought through what might break.

riversand963 · 2020-11-06T18:07:14Z

Or, we can keep unit test, but change only one line in version_set.cc as follows

diff --git a/db/version_set.cc b/db/version_set.cc
index 0fe09ef34..1d4ef1853 100644
--- a/db/version_set.cc
+++ b/db/version_set.cc
@@ -1100,9 +1100,9 @@ void LevelIterator::Seek(const Slice& target) {
     Slice file_user_key = ExtractUserKey(file_iter_.key());
     if (prefix_extractor_->InDomain(target_user_key) &&
         (!prefix_extractor_->InDomain(file_user_key) ||
-         user_comparator_.Compare(
-             prefix_extractor_->Transform(target_user_key),
-             prefix_extractor_->Transform(file_user_key)) != 0)) {
+         user_comparator_.CompareWithoutTimestamp(
+             prefix_extractor_->Transform(target_user_key), false,
+             prefix_extractor_->Transform(file_user_key), false) != 0)) {
       SetFileIterator(nullptr);
     }
   }

Then, we update the PR description so that it describes the real fix here: change Compre to CompareWithoutTimestamp.
Then, I think this PR is ready to go.

riversand963

Thanks @jay-zhuang for the fix!

facebook-github-bot · 2020-11-06T18:54:59Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

jay-zhuang · 2020-11-06T19:02:35Z

Or, we can keep unit test, but change only one line in version_set.cc as follows

diff --git a/db/version_set.cc b/db/version_set.cc
index 0fe09ef34..1d4ef1853 100644
--- a/db/version_set.cc
+++ b/db/version_set.cc
@@ -1100,9 +1100,9 @@ void LevelIterator::Seek(const Slice& target) {
     Slice file_user_key = ExtractUserKey(file_iter_.key());
     if (prefix_extractor_->InDomain(target_user_key) &&
         (!prefix_extractor_->InDomain(file_user_key) ||
-         user_comparator_.Compare(
-             prefix_extractor_->Transform(target_user_key),
-             prefix_extractor_->Transform(file_user_key)) != 0)) {
+         user_comparator_.CompareWithoutTimestamp(
+             prefix_extractor_->Transform(target_user_key), false,
+             prefix_extractor_->Transform(file_user_key), false) != 0)) {
       SetFileIterator(nullptr);
     }
   }

Then, we update the PR description so that it describes the real fix here: change Compre to CompareWithoutTimestamp.
Then, I think this PR is ready to go.

I added an unit test to cover the case that prefix extractor includes timestamp, which will return invalid result if timestamp is included. As prefix bloom filter uses prefix without timestamp, so as here.

facebook-github-bot · 2020-11-06T19:23:32Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

facebook-github-bot · 2020-11-10T19:24:21Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

facebook-github-bot · 2020-11-10T19:33:53Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

facebook-github-bot · 2020-11-10T23:37:06Z

@jay-zhuang merged this pull request in 18aee7d.

Summary: During seek, prefix compare should not include timestamp. Pull Request resolved: facebook#7644 Test Plan: added unittest Reviewed By: riversand963 Differential Revision: D24772066 Pulled By: jay-zhuang fbshipit-source-id: 3982655a8bf8da256a738e8497b73b3d9bdac92e

facebook-github-bot added the CLA Signed label Nov 5, 2020

jay-zhuang requested a review from riversand963 November 5, 2020 23:57

facebook-github-bot reviewed Nov 5, 2020

View reviewed changes

riversand963 approved these changes Nov 6, 2020

View reviewed changes

riversand963 suggested changes Nov 6, 2020

View reviewed changes

facebook-github-bot reviewed Nov 6, 2020

View reviewed changes

jay-zhuang requested a review from riversand963 November 6, 2020 21:48

riversand963 approved these changes Nov 10, 2020

View reviewed changes

jay-zhuang added 6 commits November 10, 2020 11:20

Add unittest

f2f4097

Fix

f4f71cd

Update unittest

bbefe47

Fix status assert

376eafc

Add a unittest with larger prefix extractor

6ea9798

Fix test

8cd6532

jay-zhuang force-pushed the ts_seek branch from 9c61b0d to 2278a40 Compare November 10, 2020 19:23

jay-zhuang changed the title ~~Fix a seek issue with timestamp~~ Fix a seek issue with prefix extractor and timestamp Nov 10, 2020

facebook-github-bot reviewed Nov 10, 2020

View reviewed changes

Update history

c47de7e

jay-zhuang force-pushed the ts_seek branch from 2278a40 to c47de7e Compare November 10, 2020 19:33

facebook-github-bot reviewed Nov 10, 2020

View reviewed changes

facebook-github-bot closed this in 18aee7d Nov 10, 2020

facebook-github-bot added the Merged label Nov 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a seek issue with prefix extractor and timestamp #7644

Fix a seek issue with prefix extractor and timestamp #7644

jay-zhuang commented Nov 5, 2020

facebook-github-bot left a comment

riversand963 left a comment

riversand963 Nov 6, 2020

jay-zhuang Nov 6, 2020

riversand963 Nov 6, 2020

jay-zhuang Nov 6, 2020

riversand963 Nov 6, 2020

riversand963 Nov 6, 2020

jay-zhuang Nov 6, 2020

pdillinger Nov 9, 2020

riversand963 commented Nov 6, 2020

riversand963 left a comment

facebook-github-bot commented Nov 6, 2020

jay-zhuang commented Nov 6, 2020

facebook-github-bot commented Nov 6, 2020

facebook-github-bot left a comment

facebook-github-bot commented Nov 10, 2020

facebook-github-bot left a comment

facebook-github-bot commented Nov 10, 2020

facebook-github-bot left a comment

facebook-github-bot commented Nov 10, 2020

	// db.NewIterator(). For prefix filtering to work properly,
	// "prefix_extractor" and "comparator" must be such that the following
	// properties hold:
	//
	// 1) key.starts_with(prefix(key))
	// 2) Compare(prefix(key), key) <= 0.
	// 3) If Compare(k1, k2) <= 0, then Compare(prefix(k1), prefix(k2)) <= 0
	// 4) prefix(prefix(key)) == prefix(key)

Fix a seek issue with prefix extractor and timestamp #7644

Fix a seek issue with prefix extractor and timestamp #7644

Conversation

jay-zhuang commented Nov 5, 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

Choose a reason for hiding this comment

riversand963 commented Nov 6, 2020

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 6, 2020

jay-zhuang commented Nov 6, 2020

facebook-github-bot commented Nov 6, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 10, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 10, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 10, 2020