txn: fix the ttlmanager and cleanup logic for 1pc and async commit #23342

cfzjywxk · 2021-03-16T07:22:09Z

What problem does this PR solve?

Issue Number: close #23331

Problem Summary:
There are two problems:

The left pessimistic locks are not cleaned up using 1pc protocol to commit, blocking other concurrent transactions on these keys.
The ttlManager is not closed, if the max ts caculation error is reported using async commit or 1pc, thus the left pessimistic locks could not be resolved by the ddl backfill worker.

What is changed and how it works?

What's Changed:

Do cleanup if the error is reported when 1pc protocol is used.
Close the ttlManager if the execution result of twoPhaseCommitter is error.

How it Works:

Related changes

Need to cherry-pick to the release branch

Check List

Tests

Unit test

Side effects

Release note

No release note.

cfzjywxk · 2021-03-16T07:29:24Z

store/tikv/2pc.go

@@ -920,6 +920,9 @@ func (c *twoPhaseCommitter) execute(ctx context.Context) (err error) {
 		if c.isOnePC() {
 			// The error means the 1PC transaction failed.
 			if err != nil {
+				if c.getUndeterminedErr() == nil {
+					c.cleanup(ctx)


Should we just use something like pessimisticRollbackMutations if it's pessimistic transactions instead of cleanup.

I think there won't be too much difference, but maybe we can skip this for optimistic 1pc transactions. @sticnarf How do you think?

We can always use pessimistic rollback because an optimistic one pc won't leave any waste.

How about changing cleanup()?

cleanupKeysCtx := context.WithValue(context.Background(), TxnStartKey, ctx.Value(TxnStartKey)) var err error if c.isPessimistic && c.isOnePC() { err = c.pessimisticRollbackMutations(NewBackofferWithVars(cleanupKeysCtx, cleanupMaxBackoff, c.txn.vars), c.mutations) } else { err = c.cleanupMutations(NewBackofferWithVars(cleanupKeysCtx, cleanupMaxBackoff, c.txn.vars), c.mutations) } if err != nil {

@youjiali1995 We needn't do cleanup for optimistic 1PC, so I suggest this:

if !c.isOnePC() { err = c.cleanupMutations(NewBackofferWithVars(cleanupKeysCtx, cleanupMaxBackoff, c.txn.vars), c.mutations) } else if c.isPessimistic { err = c.pessimisticRollbackMutations(NewBackofferWithVars(cleanupKeysCtx, cleanupMaxBackoff, c.txn.vars), c.mutations) }

My mistake.

MyonKeminta

LGTM

MyonKeminta · 2021-03-16T07:45:30Z

store/tikv/2pc.go

@@ -920,6 +920,9 @@ func (c *twoPhaseCommitter) execute(ctx context.Context) (err error) {
 		if c.isOnePC() {
 			// The error means the 1PC transaction failed.
 			if err != nil {
+				if c.getUndeterminedErr() == nil {
+					c.cleanup(ctx)


I think there won't be too much difference, but maybe we can skip this for optimistic 1pc transactions. @sticnarf How do you think?

ti-chi-bot · 2021-03-16T08:07:00Z

@MyonKeminta: 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.

cfzjywxk · 2021-03-18T02:02:58Z

@sticnarf @youjiali1995 PTAL

youjiali1995

Rest LGTM

cfzjywxk · 2021-03-18T03:38:16Z

store/tikv/txn.go

@@ -250,7 +250,11 @@ func (txn *KVTxn) Commit(ctx context.Context) error {
 	}
 	defer func() {
 		// For async commit transactions, the ttl manager will be closed in the asynchronous commit goroutine.
-		if !committer.isAsyncCommit() {
+		if committer.isAsyncCommit() || committer.isOnePC() {


@sticnarf @youjiali1995

Could we just always close the ttl manager here, seems at commit phase and the async commit/1pc transaction size will be small.

I think it is okay. The chance is small that TTL expires before the primary lock is committed.

I think yes because SafeWindow(2s) is smaller than the interval of keepAlive(10s). It's nearly useless here.

It seems to me not related to SafeWindow. SafeWindow only constrains the prewrite time.

sticnarf · 2021-03-18T03:52:27Z

store/tikv/2pc.go

@@ -899,15 +899,20 @@ func (c *twoPhaseCommitter) cleanup(ctx context.Context) {
 		})

 		cleanupKeysCtx := context.WithValue(context.Background(), TxnStartKey, ctx.Value(TxnStartKey))
-		err := c.cleanupMutations(NewBackofferWithVars(cleanupKeysCtx, cleanupMaxBackoff, c.txn.vars), c.mutations)
+		var err error
+		if c.isPessimistic && c.isOnePC() {


How about skipping cleanup for optimistic one pc? https://github.com/pingcap/tidb/pull/23342/files#r596528085

youjiali1995

Should also remove https://github.com/pingcap/tidb/pull/23342/files#diff-7352f99d1adcf12eded39ff8934684296725268e9116dcecfa16f560827f9271R1173

cfzjywxk · 2021-03-18T03:59:51Z

Should also remove https://github.com/pingcap/tidb/pull/23342/files#diff-7352f99d1adcf12eded39ff8934684296725268e9116dcecfa16f560827f9271R1173

@youjiali1995
Which part?

sticnarf · 2021-03-18T04:00:03Z

Should also remove https://github.com/pingcap/tidb/pull/23342/files#diff-7352f99d1adcf12eded39ff8934684296725268e9116dcecfa16f560827f9271R1173

I cannot see the line you refer to. I guess it's the defer c.ttlManager.close() in the asynchrounous commit stage?

youjiali1995 · 2021-03-18T04:00:41Z

I cannot see the line you refer to. I guess it's the defer c.ttlManager.close() in the asynchrounous commit stage?

Yes.

cfzjywxk · 2021-03-18T04:05:51Z

Should also remove https://github.com/pingcap/tidb/pull/23342/files#diff-7352f99d1adcf12eded39ff8934684296725268e9116dcecfa16f560827f9271R1173

I cannot see the line you refer to. I guess it's the defer c.ttlManager.close() in the asynchrounous commit stage?

Got, it's removed.

ti-chi-bot · 2021-03-18T04:18:52Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

MyonKeminta
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.

sticnarf

Lgtm

ti-chi-bot · 2021-03-18T04:25:46Z

@sticnarf: 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.

cfzjywxk · 2021-03-18T04:36:21Z

/merge

ti-chi-bot · 2021-03-18T04:36:24Z

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

Commit hash: 76838b2

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-03-18T04:53:40Z

cherry pick to release-5.0 in PR #23388

…23342) (#23388)

…ingcap#23342)

cfzjywxk added type/bugfix This PR fixes a bug. sig/transaction SIG:Transaction labels Mar 16, 2021

cfzjywxk requested review from you06, MyonKeminta, youjiali1995 and sticnarf March 16, 2021 07:22

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

cfzjywxk added the needs-cherry-pick-release-5.0 label Mar 16, 2021

cfzjywxk requested a review from zyguan March 16, 2021 07:24

cfzjywxk commented Mar 16, 2021

View reviewed changes

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

MyonKeminta approved these changes Mar 16, 2021

View reviewed changes

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

ichn-hu mentioned this pull request Mar 16, 2021

Welcome to contribute #20804

Closed

youjiali1995 reviewed Mar 18, 2021

View reviewed changes

cfzjywxk added 3 commits March 18, 2021 11:16

fix the ttlmanager and cleanup logic for 1pc and async commit

1a28404

change comment

c03ff28

change cleanup logic for 1pc cleanup

0c2c984

cfzjywxk force-pushed the fix_cleanup branch from df0e934 to 0c2c984 Compare March 18, 2021 03:29

cfzjywxk commented Mar 18, 2021

View reviewed changes

remove the extra logic for close

974f58e

sticnarf reviewed Mar 18, 2021

View reviewed changes

youjiali1995 reviewed Mar 18, 2021

View reviewed changes

change cleanup logic

e239169

remove the close

76838b2

youjiali1995 approved these changes Mar 18, 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 Mar 18, 2021

sticnarf approved these changes Mar 18, 2021

View reviewed changes

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

Merge branch 'master' into fix_cleanup

bfc116e

ti-chi-bot merged commit 1ab3b48 into pingcap:master Mar 18, 2021

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Mar 18, 2021

cherry pick pingcap#23342 to release-5.0

77c0ec2

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Mar 18, 2021

txn: fix the ttlmanager and cleanup logic for 1pc and async commit (#23342) #23388

Merged

ti-chi-bot pushed a commit that referenced this pull request Mar 18, 2021

txn: fix the ttlmanager and cleanup logic for 1pc and async commit (#…

375de37

…23342) (#23388)

SabaPing pushed a commit to SabaPing/tidb that referenced this pull request Mar 25, 2021

txn: fix the ttlmanager and cleanup logic for 1pc and async commit (p…

216b760

…ingcap#23342)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txn: fix the ttlmanager and cleanup logic for 1pc and async commit #23342

txn: fix the ttlmanager and cleanup logic for 1pc and async commit #23342

cfzjywxk commented Mar 16, 2021 •

edited

Loading

cfzjywxk Mar 16, 2021

MyonKeminta Mar 16, 2021

sticnarf Mar 16, 2021 •

edited

Loading

youjiali1995 Mar 18, 2021

sticnarf Mar 18, 2021

youjiali1995 Mar 18, 2021

MyonKeminta left a comment

MyonKeminta Mar 16, 2021

ti-chi-bot commented Mar 16, 2021

cfzjywxk commented Mar 18, 2021

youjiali1995 left a comment

cfzjywxk Mar 18, 2021

sticnarf Mar 18, 2021

youjiali1995 Mar 18, 2021

sticnarf Mar 18, 2021

sticnarf Mar 18, 2021

youjiali1995 left a comment

cfzjywxk commented Mar 18, 2021

sticnarf commented Mar 18, 2021

youjiali1995 commented Mar 18, 2021

cfzjywxk commented Mar 18, 2021

ti-chi-bot commented Mar 18, 2021

sticnarf left a comment

ti-chi-bot commented Mar 18, 2021

cfzjywxk commented Mar 18, 2021

ti-chi-bot commented Mar 18, 2021

ti-srebot commented Mar 18, 2021

txn: fix the ttlmanager and cleanup logic for 1pc and async commit #23342

txn: fix the ttlmanager and cleanup logic for 1pc and async commit #23342

Conversation

cfzjywxk commented Mar 16, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sticnarf Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyonKeminta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot commented Mar 16, 2021

cfzjywxk commented Mar 18, 2021

youjiali1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youjiali1995 left a comment

Choose a reason for hiding this comment

cfzjywxk commented Mar 18, 2021

sticnarf commented Mar 18, 2021

youjiali1995 commented Mar 18, 2021

cfzjywxk commented Mar 18, 2021

ti-chi-bot commented Mar 18, 2021

sticnarf left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Mar 18, 2021

cfzjywxk commented Mar 18, 2021

ti-chi-bot commented Mar 18, 2021

ti-srebot commented Mar 18, 2021

cfzjywxk commented Mar 16, 2021 •

edited

Loading

sticnarf Mar 16, 2021 •

edited

Loading