Update TransactionUtil::CheckKeyForConflict to also use timestamps #9162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request was exported from Phabricator. Differential Revision: D31567960 |
ltamasi
approved these changes
Nov 15, 2021
db/db_impl/db_impl.cc
Outdated
| @@ -4409,6 +4422,7 @@ Status DBImpl::GetLatestSequenceForKey(SuperVersion* sv, const Slice& key, | |||
|
|
|||
| if (*seq != kMaxSequenceNumber) { | |||
| // Found a sequence number, no need to check immutable memtables | |||
| assert(0 == ts_sz || *timestamp != std::string(ts_sz, '\xff')); | |||
There was a problem hiding this comment.
I think we could extend these assertions a bit (and move them out of their corresponding if blocks) to check that if we have not found a sequence number yet, the timestamp has not been updated, e.g.:
assert(!ts_sz || (*seq != kMaxSequenceNumber && *timestamp != std::string(ts_sz, '\xff')) || (*seq == kMaxSequenceNumber && *timestamp == std::string(ts_sz, '\xff')));
|
|
||
| std::string key_str; | ||
| PutFixed64(&key_str, key); | ||
| std::reverse(key_str.begin(), key_str.end()); |
There was a problem hiding this comment.
Just curious: why do we reverse the key here?
There was a problem hiding this comment.
We reverse it so that we can do memcmp on key_strs.
|
This pull request was exported from Phabricator. Differential Revision: D31567960 |
riversand963
force-pushed
the
export-D31567960
branch
from
ae4e005
to
687148a
Compare
…acebook#9162) Summary: Pull Request resolved: facebook#9162 Existing TransactionUtil::CheckKeyForConflict() performs only seq-based conflict checking. If user-defined timestamp is enabled, it should perform conflict checking based on timestamps too. Update TransactionUtil::CheckKey-related methods to verify the timestamp of the latest version of a key is smaller than the read timestamp. Note that CheckKeysForConflict() is not updated since it's used only by optimistic transaction, and we do not plan to update it in this upcoming batch of diffs. Existing GetLatestSequenceForKey() returns the sequence of the latest version of a specific user key. Since we support user-defined timestamp, we need to update this method to also return the timestamp (if enabled) of the latest version of the key. This will be needed for snapshot validation. Reviewed By: ltamasi Differential Revision: D31567960 fbshipit-source-id: 8478594f34e10e0e993255d4f715f870511d639f
|
This pull request was exported from Phabricator. Differential Revision: D31567960 |
riversand963
force-pushed
the
export-D31567960
branch
from
687148a
to
acd75e2
Compare
|
Thanks @ltamasi for the review! |
|
This pull request has been merged in 2035798. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Existing TransactionUtil::CheckKeyForConflict() performs only seq-based
conflict checking. If user-defined timestamp is enabled, it should perform
conflict checking based on timestamps too.
Update TransactionUtil::CheckKey-related methods to verify the timestamp of the
latest version of a key is smaller than the read timestamp. Note that
CheckKeysForConflict() is not updated since it's used only by optimistic
transaction, and we do not plan to update it in this upcoming batch of diffs.
Existing GetLatestSequenceForKey() returns the sequence of the latest
version of a specific user key. Since we support user-defined timestamp, we
need to update this method to also return the timestamp (if enabled) of the
latest version of the key. This will be needed for snapshot validation.
Differential Revision: D31567960