txn: fix retry with non autocommit #22875

cfzjywxk · 2021-02-22T15:08:43Z

What problem does this PR solve?

Issue Number: close #22874

Problem Summary:
The txn retry for non autocommit mode does not set flag correctly like mysql.ServerStatusInTrans.

What is changed and how it works?

What's Changed:

~~Try to make transaction activiation use the same logic for common and retrying transactions.~~
Make multiple transaction activiation interfaces use the same inner activiation logic, avoid maintaining duplicate codes.(There is still some refactor work need to do for this part, these transaction interfaces are diffuicult to distinguish and use).
~~There are three different transaction activiation interfaces, NewTxn, txn.Txn(true), initTxnWithStartTS, and they have some their own initilizing logic.~~
Above issues will be recorded as refactor tasks in the future, to lower the changing risk in release branch.
Add flag for the multiple statments transaction retry path.

How it Works:

Related changes

Check List

Tests

Unit test

Side effects

Release note

Fix the issue that transaction retry with non-autocommit mode may write incorrect data.

cfzjywxk · 2021-02-22T15:08:56Z

/run-all-tests

cfzjywxk · 2021-02-23T02:53:33Z

session/session.go

@@ -2257,6 +2250,7 @@ func (s *session) InitTxnWithStartTS(startTS uint64) error {
 	if err != nil {
 		return err
 	}
+	s.initTxn()


Will this bring some side effects?

PTAL @tiancaiamao , thx!

To lower the risk, changes to NewTxn and initTxnWithStartTS are removed.

cfzjywxk · 2021-02-23T04:20:52Z

Or shold we just change it back to use PrepareTxnCtx before retry instead of NewTxn.
For the panic issue #21284, we just add a validation check for txn and use it if it's valid at L593, to solve the panic issue.
This will reduce the change size lower the change risk.

@coocood @lysu @youjiali1995 @wjhuang2016 @jackysp

youjiali1995 · 2021-02-23T05:35:45Z

Or shold we just change it back to use PrepareTxnCtx before retry instead of NewTxn.
For the panic issue #21284, we just add a validation check for txn and use it if it's valid at L593, to solve the panic issue.
This will reduce the change size lower the change risk.

@coocood @lysu @youjiali1995 @wjhuang2016 @jackysp

Agree with it. We can merge changes of this PR to master.

coocood · 2021-02-23T05:49:09Z

Or shold we just change it back to use PrepareTxnCtx before retry instead of NewTxn.
For the panic issue #21284, we just add a validation check for txn and use it if it's valid at L593, to solve the panic issue.
This will reduce the change size lower the change risk.

@coocood @lysu @youjiali1995 @wjhuang2016 @jackysp

+1

jackysp · 2021-02-23T06:04:06Z

Or shold we just change it back to use PrepareTxnCtx before retry instead of NewTxn.
For the panic issue #21284, we just add a validation check for txn and use it if it's valid at L593, to solve the panic issue.
This will reduce the change size lower the change risk.

@coocood @lysu @youjiali1995 @wjhuang2016 @jackysp

The set statement should be retried in the same transaction with other history statements. If it is the first statement of the history and the session only prepared a transaction but didn't active it, the set statement may make the retry results not as expected. e.g.

set autocommit = 0;
select * from t;
set @a=@@tidb_current_ts;
update t set startts = @a where xxx;
commit;

If the above transaction retried, the column startts will be set to 0, because some variables are transaction related.

cfzjywxk · 2021-02-23T07:38:33Z

/run-all-tests

coocood · 2021-02-23T07:42:20Z

LGTM

ti-chi-bot · 2021-02-23T07:42:21Z

@coocood: Please use /LGTM instead of LGTM when you want to approve the pull request by comment.
If you use the GitHub review feature, please approve the PR directly, the comment will not take effect in the GitHub review feature.
If you have any qustions please refer to lgtm command help or lgtm plugin design.

If you have approved this PR, please ignore this reply. This reply is being used as a temporary reply during the migration of the new bot and will be removed on April 1.

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.

coocood · 2021-02-23T07:42:29Z

/LGTM

session/session.go

AilinKid

LGTM

ti-chi-bot · 2021-02-23T08:08:45Z

@AilinKid: Please use /LGTM instead of LGTM when you want to approve the pull request by comment.
If you use the GitHub review feature, please approve the PR directly, the comment will not take effect in the GitHub review feature.
If you have any qustions please refer to lgtm command help or lgtm plugin design.

If you have approved this PR, please ignore this reply. This reply is being used as a temporary reply during the migration of the new bot and will be removed on April 1.

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.

AilinKid · 2021-02-23T08:52:45Z

/LGTM

cfzjywxk · 2021-02-23T09:00:46Z

/run-all-tests

session/session.go

jackysp

LGTM

ti-chi-bot · 2021-02-23T09:50:11Z

@jackysp: Please use /LGTM instead of LGTM when you want to approve the pull request by comment.
If you use the GitHub review feature, please approve the PR directly, the comment will not take effect in the GitHub review feature.
If you have any qustions please refer to lgtm command help or lgtm plugin design.

If you have approved this PR, please ignore this reply. This reply is being used as a temporary reply during the migration of the new bot and will be removed on April 1.

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.

youjiali1995

/lgtm

cfzjywxk · 2021-02-23T09:56:12Z

/run-all-tests

youjiali1995 · 2021-02-23T10:54:51Z

/LGTM

ti-chi-bot · 2021-02-23T10:54:54Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

AilinKid
coocood
youjiali1995

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

jackysp · 2021-02-23T10:58:41Z

/LGTM

jebter · 2021-02-23T13:13:58Z

/label cherry-pick-approved

jebter · 2021-02-24T00:36:33Z

/merge

ti-chi-bot · 2021-02-24T00:36:34Z

@jebter: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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.

ti-chi-bot · 2021-02-24T00:37:07Z

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

Commit hash: 8f8d450

cfzjywxk added type/bugfix This PR fixes a bug. priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/transaction SIG:Transaction labels Feb 22, 2021

cfzjywxk added this to the v4.0.11 milestone Feb 22, 2021

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 22, 2021

github-actions bot added the sig/sql-infra SIG: SQL Infra label Feb 22, 2021

cfzjywxk requested review from wjhuang2016, youjiali1995, jackysp, coocood and lysu February 23, 2021 02:18

cfzjywxk added the require-LGT3 Indicates that the PR requires three LGTM. label Feb 23, 2021

cfzjywxk commented Feb 23, 2021

View reviewed changes

sticnarf mentioned this pull request Feb 23, 2021

session: fix linearizability for non-autocommit async-commit txn #22879

Merged

fix the transaction flag settings doing auto retry

d4cbe9d

cfzjywxk force-pushed the fix_retry_with_non_autocommit branch from ef78bfc to d4cbe9d Compare February 23, 2021 07:34

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 Feb 23, 2021

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

jackysp reviewed Feb 23, 2021

View reviewed changes

session/session.go Outdated Show resolved Hide resolved

AilinKid reviewed Feb 23, 2021

View reviewed changes

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 Feb 23, 2021

remove multiple statement retry pessimistic config check

5f12760

youjiali1995 reviewed Feb 23, 2021

View reviewed changes

session/session.go Outdated Show resolved Hide resolved

always set flag for multiple statement retry

8f8d450

jackysp reviewed Feb 23, 2021

View reviewed changes

youjiali1995 reviewed Feb 23, 2021

View reviewed changes

ti-chi-bot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Feb 23, 2021

jebter added the cherry-pick-approved Cherry pick PR approved by release team. label Feb 23, 2021

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

ti-chi-bot merged commit 8e68350 into pingcap:release-4.0 Feb 24, 2021

This was referenced Mar 1, 2021

txn: non autocommit mode transaction retry will get incorrect result #22874

Closed

txn: the activation does not work as expected for some cases #23015

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txn: fix retry with non autocommit #22875

txn: fix retry with non autocommit #22875

cfzjywxk commented Feb 22, 2021 •

edited

Loading

cfzjywxk commented Feb 22, 2021

cfzjywxk Feb 23, 2021

jackysp Feb 23, 2021

cfzjywxk Feb 23, 2021

cfzjywxk commented Feb 23, 2021

youjiali1995 commented Feb 23, 2021

coocood commented Feb 23, 2021

jackysp commented Feb 23, 2021 •

edited

Loading

cfzjywxk commented Feb 23, 2021

coocood commented Feb 23, 2021

ti-chi-bot commented Feb 23, 2021

coocood commented Feb 23, 2021

AilinKid left a comment

ti-chi-bot commented Feb 23, 2021

AilinKid commented Feb 23, 2021

cfzjywxk commented Feb 23, 2021

jackysp left a comment

ti-chi-bot commented Feb 23, 2021

youjiali1995 left a comment

cfzjywxk commented Feb 23, 2021

youjiali1995 commented Feb 23, 2021

ti-chi-bot commented Feb 23, 2021

jackysp commented Feb 23, 2021

jebter commented Feb 23, 2021

jebter commented Feb 24, 2021

ti-chi-bot commented Feb 24, 2021

ti-chi-bot commented Feb 24, 2021

txn: fix retry with non autocommit #22875

txn: fix retry with non autocommit #22875

Conversation

cfzjywxk commented Feb 22, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

cfzjywxk commented Feb 22, 2021

cfzjywxk Feb 23, 2021

Choose a reason for hiding this comment

jackysp Feb 23, 2021

Choose a reason for hiding this comment

cfzjywxk Feb 23, 2021

Choose a reason for hiding this comment

cfzjywxk commented Feb 23, 2021

youjiali1995 commented Feb 23, 2021

coocood commented Feb 23, 2021

jackysp commented Feb 23, 2021 • edited Loading

cfzjywxk commented Feb 23, 2021

coocood commented Feb 23, 2021

ti-chi-bot commented Feb 23, 2021

coocood commented Feb 23, 2021

AilinKid left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Feb 23, 2021

AilinKid commented Feb 23, 2021

cfzjywxk commented Feb 23, 2021

jackysp left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Feb 23, 2021

youjiali1995 left a comment

Choose a reason for hiding this comment

cfzjywxk commented Feb 23, 2021

youjiali1995 commented Feb 23, 2021

ti-chi-bot commented Feb 23, 2021

jackysp commented Feb 23, 2021

jebter commented Feb 23, 2021

jebter commented Feb 24, 2021

ti-chi-bot commented Feb 24, 2021

ti-chi-bot commented Feb 24, 2021

cfzjywxk commented Feb 22, 2021 •

edited

Loading

jackysp commented Feb 23, 2021 •

edited

Loading