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

executor: reject setting read ts to a future time #25732

Merged
merged 16 commits into from
Jun 25, 2021
Merged

executor: reject setting read ts to a future time #25732

merged 16 commits into from
Jun 25, 2021

Conversation

sticnarf
Copy link
Contributor

@sticnarf sticnarf commented Jun 24, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: close #25680

What is changed and how it works?

This PR checks the read ts in two cases:

  • Set by tidb_snapshot system variable
  • Stale read

It first checks with latest local timestamp (with low cost). If the check fails, it gets a timestamp from PD to do a double check.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

  • Performance regression
    • Consumes more CPU
  • Breaking backward compatibility

Release note

  • Do not allow setting read timestamp to a future time.

@sticnarf
executor: reject a future tidb_snapshot session variable
dd7c613 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@ti-chi-bot ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 24, 2021
@github-actions github-actions bot added the sig/execution SIG execution label Jun 24, 2021
sticnarf added 2 commits June 24, 2021 14:49
@sticnarf
add unit test
e8daeba 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
refmt
60b8961 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf marked this pull request as ready for review June 24, 2021 06:56
@sticnarf sticnarf requested a review from a team as a code owner June 24, 2021 06:56
@sticnarf sticnarf requested review from qw4990 and removed request for a team June 24, 2021 06:56
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2021
@sticnarf sticnarf requested a review from youjiali1995 June 24, 2021 06:56
@sticnarf sticnarf requested a review from NingLin-P June 24, 2021 07:01
@sticnarf
Copy link
Contributor Author

cc @NingLin-P
Does the timestamp of stale read need to be checked? I don't know whether "stale read" with a future time must be supported.

@youjiali1995 youjiali1995 requested a review from cfzjywxk June 24, 2021 07:19
@NingLin-P
Copy link
Contributor

cc @NingLin-P
Does the timestamp of stale read need to be checked? I don't know whether "stale read" with a future time must be supported.

I think stale read need to check too, using stale read with a future time seems have no use case and benefit. /cc @nolouch

@nolouch
Copy link
Member

nolouch commented Jun 24, 2021 •
edited
Loading

cc @NingLin-P
Does the timestamp of stale read need to be checked? I don't know whether "stale read" with a future time must be supported.

I think stale read need to check too, using stale read with a future time seems have no use case and benefit. /cc @nolouch

Better to check, but I think the resolution 2s of GetLowResolutionTimestamp may not suitable to stale read because stale read supports Bound Read, which means user want to read new data as soon as possible. or users may read data withNow() - Interval 1 Second although we recommend the best practice is around Now() - Interval 5 second.

@sticnarf
also check stale read ts
bb0315f 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf requested a review from a team as a code owner June 24, 2021 09:03
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 24, 2021
@ichn-hu ichn-hu mentioned this pull request Jun 24, 2021
Closed
nolouch
nolouch reviewed Jun 24, 2021
View reviewed changes
sessionctx/context.go Outdated Show resolved Hide resolved
nolouch
nolouch reviewed Jun 24, 2021
View reviewed changes
executor/set.go Outdated Show resolved Hide resolved
nolouch
nolouch reviewed Jun 24, 2021
View reviewed changes
Copy link
Member

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

rest LGTM

sessionctx/context.go Outdated Show resolved Hide resolved
@nolouch nolouch requested a review from Yisaer June 24, 2021 13:48
Yisaer
Yisaer reviewed Jun 25, 2021
View reviewed changes
Copy link
Contributor

@Yisaer Yisaer left a comment

Choose a reason for hiding this comment

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

Rest LGTM

@youjiali1995
Copy link
Contributor

@Yisaer PTAL again, thanks!

@Yisaer again...

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Yisaer
  • nolouch

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 25, 2021
@Yisaer
Copy link
Contributor

Yisaer commented Jun 25, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 0a93d8c

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 25, 2021
@youjiali1995
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

@youjiali1995: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@sticnarf
Copy link
Contributor Author

/rebuild

sticnarf added a commit to sticnarf/tidb that referenced this pull request Jun 25, 2021
@sticnarf
executor: reject setting read ts to a future time (pingcap#25732)
918883a 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf mentioned this pull request Jun 25, 2021
Merged
@sticnarf
Copy link
Contributor Author

/run-check_dev_2

@ti-chi-bot ti-chi-bot merged commit 92ddceb into pingcap:master Jun 25, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 25, 2021
@sticnarf @ti-srebot
cherry pick pingcap#25732 to release-5.0
10b351c 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jun 25, 2021
Closed
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #25762

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 25, 2021
@sticnarf @ti-srebot
cherry pick pingcap#25732 to release-5.1
4199a79 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jun 25, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-5.1 in PR #25763

ti-chi-bot pushed a commit that referenced this pull request Jun 25, 2021
@sticnarf
executor: reject setting read ts to a future time (#25732) (#25761)
64b7ecd
jiajunhuang pushed a commit to jiajunhuang/tidb that referenced this pull request Jun 28, 2021
@sticnarf @jiajunhuang
executor: reject setting read ts to a future time (pingcap#25732)
db0da0f
ti-chi-bot pushed a commit that referenced this pull request Jul 20, 2021
@ti-srebot
executor: reject setting read ts to a future time (#25732) (#25763)
b1f47e7
@bb7133 bb7133 mentioned this pull request Jan 14, 2022
Open
@cfzjywxk cfzjywxk mentioned this pull request Apr 11, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youjiali1995 youjiali1995 youjiali1995 approved these changes

@Yisaer Yisaer Yisaer approved these changes

@nolouch nolouch nolouch approved these changes

@NingLin-P NingLin-P Awaiting requested review from NingLin-P

@cfzjywxk cfzjywxk Awaiting requested review from cfzjywxk

Assignees
No one assigned
Labels
needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 sig/execution SIG execution size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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.

Don't allow reading a snapshot with the timestamp greater than the current max timestamp
9 participants
@sticnarf @NingLin-P @nolouch @cfzjywxk @ti-chi-bot @youjiali1995 @sre-bot @Yisaer @ti-srebot