executor: implement set transaction read only as of transaction by Yisaer · Pull Request #24766 · pingcap/tidb

Yisaer · 2021-05-19T13:30:19Z

Signed-off-by: yisaer disxiaofei@163.com

What problem does this PR solve?

Issue Number: ref #21094

What is changed and how it works?

Implement Set Transaction read only as of ..... statement.

Check List

Tests

Unit test

Release note

No release note

Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer · 2021-05-19T13:31:20Z

/cc @xhebox

nolouch · 2021-05-19T16:17:50Z

executor/stale_txn_test.go

+
+func (s *testStaleTxnSerialSuite) TestSetTransactionReadOnlyAsOf(c *C) {
+	tk := testkit.NewTestKit(c, s.store)
+	tk.MustExec(`SET TRANSACTION READ ONLY as of timestamp '2021-04-21 00:42:12'`)


Should support expression? our bound read is a function call.

It is already supported, the testcase updated.

Signed-off-by: yisaer <disxiaofei@163.com>

sessionctx/variable/sysvar.go

xhebox · 2021-05-21T01:50:18Z

/lgtm

ti-chi-bot · 2021-05-21T01:50:20Z

@xhebox: Thanks for your review. The bot only counts /lgtm from reviewers and higher roles in list, but you're still welcome to leave your comments.

Details

In response to this:

/lgtm

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.

…action_read_only_as_of

Yisaer · 2021-05-21T05:42:03Z

/label sig/sql-infra

ti-chi-bot · 2021-05-21T05:42:04Z

@Yisaer: The label(s) sig/sql-infra cannot be applied. These labels are supported: CHANGELOG, challenge-program, compatibility-breaker, contribution, correctness, first-time-contributor, good-first-issue, hacktoberfest, high-performance, hptc, need-more-info, needs-cherry-pick-3.0, needs-cherry-pick-3.1, needs-cherry-pick-4.0, needs-cherry-pick-5.0, proposal, release-note, require-LGT3, security, tools.

Details

In response to this:

/label sig/sql-infra

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.

Yisaer · 2021-05-21T05:52:24Z

/sig sql-infra

JmPotato

The rest LGTM.

JmPotato · 2021-05-21T06:03:31Z

sessionctx/variable/varsutil.go

+}
+
 // GoTimeToTS converts a Go time to uint64 timestamp.
 func GoTimeToTS(t time.Time) uint64 {


I think this function is duplicated with oracle.GoTimeToTS, maybe we should remove it and use the latter.

Signed-off-by: yisaer <disxiaofei@163.com> fix test Signed-off-by: yisaer <disxiaofei@163.com>

xhebox · 2021-05-24T05:10:56Z

/lgtm

djshow832 · 2021-05-25T06:41:14Z

executor/simple.go

+		e.ctx.GetSessionVars().SetInTxn(true)
+		return nil


Why do you set it and return? To walk around s.ReadOnly part?

djshow832 · 2021-05-25T06:43:03Z

sessionctx/variable/sysvar.go

 	}, GetSession: func(s *SessionVars) (string, error) {
 		return s.TxnScope.GetVarValue(), nil
 	}},
+	{Scope: ScopeSession, Name: TiDBTxnReadTS, Value: "", SetSession: func(s *SessionVars, val string) error {


Is this sysvar exposed to users?

I think it is ok to exposed it.

The problem is, users may also use this variable to set read ts.

Signed-off-by: yisaer <disxiaofei@163.com> address the comment Signed-off-by: yisaer <disxiaofei@163.com> address the comment Signed-off-by: yisaer <disxiaofei@163.com>

…action_read_only_as_of

executor/simple.go

Signed-off-by: yisaer <disxiaofei@163.com> address the comment Signed-off-by: yisaer <disxiaofei@163.com>

djshow832 · 2021-05-25T07:58:28Z

/lgtm

ti-chi-bot · 2021-05-25T07:58:31Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

djshow832
xhebox

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.

Details

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

djshow832 · 2021-05-25T07:59:37Z

/merge

ti-chi-bot · 2021-05-25T07:59:40Z

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

Details

Commit hash: 6c938a7

Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer · 2021-05-25T08:11:10Z

/merge

ti-chi-bot · 2021-05-25T08:11:13Z

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

Details

Commit hash: 3c465d0

ti-chi-bot · 2021-05-25T08:29:44Z

@Yisaer: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

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.

support set transaction

72498dd

Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer requested a review from a team as a code owner May 19, 2021 13:30

Yisaer requested review from XuHuaiyu and removed request for a team May 19, 2021 13:30

ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 19, 2021

ti-chi-bot requested a review from xhebox May 19, 2021 13:31

github-actions bot added the sig/execution SIG execution label May 19, 2021

nolouch reviewed May 19, 2021

View reviewed changes

address the comment

21871cd

Signed-off-by: yisaer <disxiaofei@163.com>

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 20, 2021

morgo reviewed May 20, 2021

View reviewed changes

sessionctx/variable/sysvar.go Outdated Show resolved Hide resolved

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2021

Merge remote-tracking branch 'upstream/master' into support_set_trans…

7f7643f

…action_read_only_as_of

Yisaer force-pushed the support_set_transaction_read_only_as_of branch from 97b2c43 to 7f7643f Compare May 21, 2021 05:34

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2021

ti-chi-bot added the sig/sql-infra SIG: SQL Infra label May 21, 2021

JmPotato reviewed May 21, 2021

View reviewed changes

Yisaer requested a review from nolouch May 21, 2021 10:05

fix test

8e32482

Signed-off-by: yisaer <disxiaofei@163.com> fix test Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer force-pushed the support_set_transaction_read_only_as_of branch from b320672 to 8e32482 Compare May 21, 2021 10:09

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 24, 2021

JmPotato approved these changes May 24, 2021

View reviewed changes

djshow832 reviewed May 25, 2021

View reviewed changes

Yisaer force-pushed the support_set_transaction_read_only_as_of branch from 54308ce to 8085f85 Compare May 25, 2021 07:46

address the comment

ce2df75

Signed-off-by: yisaer <disxiaofei@163.com> address the comment Signed-off-by: yisaer <disxiaofei@163.com> address the comment Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer force-pushed the support_set_transaction_read_only_as_of branch from 8085f85 to ce2df75 Compare May 25, 2021 07:47

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2021

Merge remote-tracking branch 'upstream/master' into support_set_trans…

5c4ffda

…action_read_only_as_of

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2021

djshow832 reviewed May 25, 2021

View reviewed changes

executor/simple.go Outdated Show resolved Hide resolved

address the comment

6c938a7

Signed-off-by: yisaer <disxiaofei@163.com> address the comment Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer force-pushed the support_set_transaction_read_only_as_of branch from 2cb8e67 to 6c938a7 Compare May 25, 2021 07:56

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 25, 2021

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 May 25, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 25, 2021

fix test

3c465d0

Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer force-pushed the support_set_transaction_read_only_as_of branch from e2b4d41 to 3c465d0 Compare May 25, 2021 08:10

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label May 25, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 25, 2021

ti-chi-bot added 2 commits May 25, 2021 16:26

Merge branch 'master' into support_set_transaction_read_only_as_of

29b8389

Merge branch 'master' into support_set_transaction_read_only_as_of

a72892a

ti-chi-bot merged commit 6e9d275 into pingcap:master May 25, 2021

nolouch mentioned this pull request May 25, 2021

Support timestamp bounded read-only transactions on data replicas Tasklist #21094

Closed

45 tasks

Conversation

Yisaer commented May 19, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

Yisaer commented May 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xhebox commented May 21, 2021

Uh oh!

ti-chi-bot commented May 21, 2021

Uh oh!

Yisaer commented May 21, 2021

Uh oh!

ti-chi-bot commented May 21, 2021

Uh oh!

Yisaer commented May 21, 2021

Uh oh!

JmPotato left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhebox commented May 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yisaer May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

djshow832 commented May 25, 2021

Uh oh!

ti-chi-bot commented May 25, 2021

Uh oh!

djshow832 commented May 25, 2021

Uh oh!

ti-chi-bot commented May 25, 2021

Uh oh!

Yisaer commented May 25, 2021

Uh oh!

ti-chi-bot commented May 25, 2021

Uh oh!

ti-chi-bot commented May 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Yisaer May 25, 2021 •

edited

Loading