-
Notifications
You must be signed in to change notification settings - Fork 216
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
Add for_update_ts check to PrewriteRequest #1096
Add for_update_ts check to PrewriteRequest #1096
Conversation
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Maybe it would be better to rename |
The reason of allowing "<" is that it can guarantee there are no not-detected write conflict. The detail was discussed in the issue tikv/tikv#14311 . Does that make sense to you? |
Yeah I've checked that. |
@cfzjywxk PTAL, how's your opinion? |
Permitting If testing reveals scenarios that were not previously considered and may result in a significant number of replayed pessimistic locks, maybe we could take them into further consideration. |
@@ -120,6 +128,9 @@ message PrewriteRequest { | |||
uint64 max_commit_ts = 14; | |||
// The level of assertion to use on this prewrte request. | |||
AssertionLevel assertion_level = 15; | |||
// for_update_ts constriants that should be checked when prewriting a pessimistic transaction. | |||
// See https://github.com/tikv/tikv/issues/14311 | |||
repeated ForUpdateTSConstraint for_update_ts_constraints = 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the fair lock mode supports only single key lock now, does it mean for a transaction the maximum length of this array is txn.total_statment_number
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It's possible that a statement calls LockKeys
more than once and locks one key in each call.
Ok let's ignore the "<" case and only allow exactly equal. I'll change the name back to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ref: tikv/tikv#14311
Naming suggestions are welcome...