Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DBImpl::GetLatestSequenceForKey() for Merge #10724

Closed
wants to merge 3 commits into from

Conversation

riversand963
Copy link
Contributor

Summary:
Currently, without this fix, DBImpl::GetLatestSequenceForKey() may not return the latest sequence number for merge operands of the key. This can cause conflict checking during optimistic transaction commit phase to fail. Fix it by always returning the latest sequence number of the key, also considering range tombstones.

Test Plan:
make check

@facebook-github-bot
Copy link
Contributor

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

Copy link
Member

@cbi42 cbi42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@riversand963
Copy link
Contributor Author

Thanks @cbi42 for the review.
I think this should be applied to the logic of updating timestamps. It's not a problem for now because Merge does not support timestamp yet, but it is not difficult to do so. Let me add that and update a few unit tests.

@facebook-github-bot
Copy link
Contributor

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

Summary:
Currently, without this fix, DBImpl::GetLatestSequenceForKey() may not
return the latest sequence number for merge operands of the key. This
can cause conflict checking during optimistic transaction commit phase
to fail. Fix it by always returning the latest sequence number of the
key, also considering range tombstones.

Test Plan:
make check
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@riversand963 riversand963 deleted the optimistic-txn-merge branch September 24, 2022 01:31
riversand963 added a commit to riversand963/rocksdb that referenced this pull request Sep 26, 2022
riversand963 added a commit that referenced this pull request Sep 26, 2022
Summary:
Currently, without this fix, DBImpl::GetLatestSequenceForKey() may not return the latest sequence number for merge operands of the key. This can cause conflict checking during optimistic transaction commit phase to fail. Fix it by always returning the latest sequence number of the key, also considering range tombstones.

Pull Request resolved: #10724

Test Plan: make check

Reviewed By: cbi42

Differential Revision: D39756847

Pulled By: riversand963

fbshipit-source-id: 0764c3dd4cb24960b37e18adccc6e7feed0e6876
facebook-github-bot pushed a commit that referenced this pull request Sep 26, 2022
Summary: Pull Request resolved: #10737

Reviewed By: cbi42

Differential Revision: D39825386

Pulled By: riversand963

fbshipit-source-id: a3c55f2777e034d6ae6ff44ef0219d9fbbf1cc96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants