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.

Sign up for GitHub

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

Jump to bottom

store/tikv: add sanity check for snapshot version and forUpdateTS #20159

Merged
merged 7 commits into from
Sep 27, 2020
Merged

store/tikv: add sanity check for snapshot version and forUpdateTS #20159

merged 7 commits into from
Sep 27, 2020

Conversation

you06
Copy link
Contributor

@you06 you06 commented Sep 23, 2020 •
edited
Loading

Signed-off-by: you06 you1474600@gmail.com

What problem does this PR solve?

Issue Number: #19887

Problem Summary:

The invalid startTS caused TiDB stack overflow in BR's test, but we didn't know where the invalid startTS comes from, this PR add sanity check for startTS == MaxInt64.

What is changed and how it works?

What's Changed: check startTS before txn commit.

How it Works: if the startTS is an invalid value, an error will be returned and we can find where it comes from by stack trace.

Related changes

  • Need to cherry-pick to the release branch 4.0

Check List

Tests

  • Manual test (add detailed scripts or steps below)

No test is required for this change

Side effects

  • n/a

Release note

  • No release note

@you06 you06 requested a review from disksing September 23, 2020 03:12
@sre-bot
Copy link
Contributor

sre-bot commented Sep 23, 2020

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

@you06
Copy link
Contributor Author

you06 commented Sep 23, 2020

/rebuild

@you06
Copy link
Contributor Author

you06 commented Sep 23, 2020

/run-check-release-note

@sticnarf
Copy link
Contributor

But MaxInt64 is not a special value for transaction. Although it's not possible in practice that a real TS could be MaxInt64, this change still seems a bit weird.

@you06
Copy link
Contributor Author

you06 commented Sep 23, 2020

But MaxInt64 is not a special value for transaction. Although it's not possible in practice that a real TS could be MaxInt64, this change still seems a bit weird.

Agree. Another way is limiting the retry time to avoid stack overflow, but I'm afraid it'll lead to a great change.

@you06 you06 changed the base branch from master to release-4.0 September 23, 2020 06:04
@jackysp
Copy link
Member

jackysp commented Sep 23, 2020

But MaxInt64 is not a special value for transaction. Although it's not possible in practice that a real TS could be MaxInt64, this change still seems a bit weird.

It's a particularly long time ago. I think we can add such a shelf life to TiDB. :)

@you06
Copy link
Contributor Author

you06 commented Sep 23, 2020

But MaxInt64 is not a special value for transaction. Although it's not possible in practice that a real TS could be MaxInt64, this change still seems a bit weird.

It's a particularly long time ago. I think we can add such a shelf life to TiDB. :)

I'll try reproducing this first.

@disksing
Copy link
Contributor

Maybe we can treat all timestamps that greater than a certain value as invalid.

@you06
check for startTS in scan
fe21769 
Signed-off-by: you06 <you1474600@gmail.com>
@you06
add sanity check for snapshot version
ce984ba 
Signed-off-by: you06 <you1474600@gmail.com>
@you06
fmt code
58515af 
Signed-off-by: you06 <you1474600@gmail.com>
@you06
check forUpdateTS
070a8d5 
Signed-off-by: you06 <you1474600@gmail.com>
@you06 you06 changed the base branch from release-4.0 to master September 27, 2020 03:53
@you06 you06 requested review from a team as code owners September 27, 2020 03:53
@you06 you06 requested review from SunRunAway and removed request for a team September 27, 2020 03:53
@you06 you06 changed the title store/tikv: update sanity check for startTS in 2pc store/tikv: add sanity check for snapshot version and forUpdateTS Sep 27, 2020
@you06
Copy link
Contributor Author

you06 commented Sep 27, 2020

Maybe we can treat all timestamps that greater than a certain value as invalid.

I treat the snapshot with version >= MaxInt64 and not equal MaxUint64 as invalid, and check forUpdateTS to avoid too deep recursive.

@you06
panic for snapshot with invalid ts
e1dd0a9 
Signed-off-by: you06 <you1474600@gmail.com>
@you06
Copy link
Contributor Author

you06 commented Sep 27, 2020

/run-all-tests

disksing
disksing previously approved these changes Sep 27, 2020
View reviewed changes
@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 27, 2020
@you06
add an extra check for err message from TiKV
4cda500 
Signed-off-by: you06 <you1474600@gmail.com>
@you06
Copy link
Contributor Author

you06 commented Sep 27, 2020

/run-all-tests

@you06
Copy link
Contributor Author

you06 commented Sep 27, 2020

@disksing I add an extra check for the error response to avoid the infinite retry.

@you06 you06 added sig/transaction SIG:Transaction and removed status/DNM labels Sep 27, 2020
jackysp
jackysp reviewed Sep 27, 2020
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 27, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 27, 2020
@jackysp
Copy link
Member

jackysp commented Sep 27, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 27, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit f1f72d5 into pingcap:master Sep 27, 2020
@you06 you06 deleted the sanity-check branch September 27, 2020 07:39
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Sep 27, 2020
@you06 @ti-srebot
cherry pick pingcap#20159 to release-4.0
8e1fe1f 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Sep 27, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #20248

zz-jason pushed a commit that referenced this pull request Oct 3, 2020
@ti-srebot
store/tikv: add sanity check for snapshot version and forUpdateTS (#2…
6e5c0c9 
…0159) (#20248)
@andylokandy andylokandy mentioned this pull request Nov 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackysp jackysp jackysp left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

@disksing disksing disksing left review comments

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

Assignees
No one assigned
Labels
sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@you06 @sre-bot @sticnarf @jackysp @disksing @ti-srebot