txn: unify the management of transaction activation by TxnManager. #35679

SpadeA-Tang · 2022-06-23T07:21:03Z

What problem does this PR solve?

Issue Number: close #35705, #35686

Problem Summary:

Now, we can activate transaction by calling Txn as well as activateTxn by the relevant transaction context provider.
activateTxn calls Txn internally. This PR reverses this call relationship, which means it lets Txn call activateTxn internally. The reason is that we should let the txnManager to manage the transaction related initialization and modification. Currently, if someone calls Txn directly, txnManager will not be involved with it.

This PR adds a new method for the interface TxnManager: ActivateTxn, which calls the relevant transaction context provider's ActivateTxn.
And it also transfers the transaction related intialization from Txn to ActivateTxn.

By the way, it solves the problem in #35686.

What is changed and how it works?

Check List

Tests

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot · 2022-06-23T07:21:04Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lcwangchao
you06

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.

kv/kv.go

session/session.go

session/txn.go

lcwangchao · 2022-06-23T07:37:36Z

sessiontxn/isolation/base.go

+		sessVars.SetInTxn(true)
+	}
+
+	sessVars.TxnCtx.CouldRetry = sessiontxn.IsTxnRetryable(sessVars)


TxnCtx.CouldRetry can only be true when it is a optimistic txn, so it's better to move set it in OptimisticTxnContextProvider.onTxnActive, for other providers, keep it always false.

BTW, session.getSnapshotInterceptor can be removed

Good suggestion. I will modify it.

lcwangchao · 2022-06-23T07:41:22Z

sessiontxn/staleread/provider.go

@@ -90,6 +91,11 @@ func (p *StalenessTxnContextProvider) OnStmtStart(_ context.Context) error {
 	return nil
 }

+// ActivateTxn activates the transaction.
+func (p *StalenessTxnContextProvider) ActivateTxn() (kv.Transaction, error) {
+	return nil, nil


I think we should also support ActivateTxn for StalenessTxnContextProvider

Ok, I have added some code for this.

sessiontxn/txn.go

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

sre-bot · 2022-06-27T11:57:28Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/8ffb6eb2b70805b43f865d66412d6c0e07fe0cf4

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

lcwangchao · 2022-06-28T02:59:33Z

session/schema_amender_test.go

 			require.NoError(t, err)
-			se.txn.changeInvalidToValid(txn)
-			txn, err = se.Txn(true)
+			txn, err := se.Txn(false)


Why Txn(true) => Txn(false)

The txn now is already actiavated in sessiontxn.NewTxn(ctx, se), so whether true or false here has no difference.

lcwangchao · 2022-06-28T03:11:59Z

sessiontxn/isolation/base.go

 	if err != nil {
 		return nil, err
 	}

 	sessVars := p.sctx.GetSessionVars()
 	sessVars.TxnCtx.StartTS = txn.StartTS()

+	if !sessVars.IsAutocommit() && sessVars.SnapshotTS == 0 {


When EnterNewTxnDefault and autocommit=0 it will also enter an explicit txn? We should keep the behavior the same with the old implement. And we need to add more tests for this case.

I have modified it.

session/txnmanager.go

lcwangchao · 2022-06-28T03:20:46Z

domain/domain.go

@@ -326,6 +326,10 @@ func canSkipSchemaCheckerDDL(tp model.ActionType) bool {

 // InfoSchema gets the latest information schema from domain.
 func (do *Domain) InfoSchema() infoschema.InfoSchema {
+	if do.infoCache == nil {


If mockContext can return a valid infoschema, can we revert this line?

It is still needed as some tests does not init domain propoerly.

lcwangchao · 2022-06-28T03:29:39Z

sessiontxn/isolation/optimistic.go

+// If the session is already in transaction, enable retry or internal SQL could retry.
+// If not, the transaction could always retry, because it should be auto committed transaction.
+// Anyway the retry limit is 0, the transaction could not retry.
+func isTxnRetryable(sessVars *variable.SessionVars, tp *sessiontxn.EnterNewTxnType) bool {


isTxnRetryable => isOptimisticTxnRetryable may be better because it is only used by optimistic mode.

sessiontxn/staleread/provider.go

lcwangchao · 2022-06-28T03:34:59Z

sessiontxn/staleread/provider.go

+		return p.txn, nil
+	}
+
+	err := p.OnInitialize(p.ctx, sessiontxn.EnterNewTxnDefault)


Why call p.OnInitialize here? p.OnInitialize should have been called already.

Okay, I have adjusted the strcuture of this.

lcwangchao · 2022-06-28T03:38:39Z

sessiontxn/staleread/provider.go

+		return nil, err
+	}
+
+	txnFuture := p.sctx.GetPreparedTxnFuture()


You should call NewStaleTxnWithStartTS because stale read has some it's own states to set.

sessiontxn/staleread/staleread_test.go

lcwangchao · 2022-06-28T03:40:41Z

util/mock/context.go

 }

 type wrapTxn struct {
 	kv.Transaction
 }

+func (txn *wrapTxn) Wait(_ context.Context, _ sessionctx.Context) (kv.Transaction, error) {
+	return txn.Transaction, nil


Why return txn.Transaction not txn ?

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

…CallManager

lcwangchao · 2022-06-28T09:22:48Z

executor/stale_txn_test.go

+
+	tk := testkit.NewTestKit(t, store)
+	// This query should not panic
+	tk.MustExec("select * from information_schema.ddl_jobs as of timestamp now()")


select should use MustQuery

I think they are the same in this case. Anyway, I will change it.

lcwangchao · 2022-06-28T09:24:11Z

sessionctx/context.go

@@ -176,6 +176,12 @@ type Context interface {
 	ReleaseAllAdvisoryLocks() int
 }

+// TxnFuture todo: add comments


You can add comments now...

lcwangchao · 2022-06-28T09:25:52Z

sessiontxn/isolation/base.go

@@ -51,6 +52,7 @@ type baseTxnContextProvider struct {
 	infoSchema    infoschema.InfoSchema
 	txn           kv.Transaction
 	isTxnPrepared bool
+	tp            *sessiontxn.EnterNewTxnType


Why it is a pointer? And the name enterNewTxnType may be better because it is not provider's type

lcwangchao · 2022-06-28T09:27:17Z

sessiontxn/isolation/optimistic.go

+// If the session is already in transaction, enable retry or internal SQL could retry.
+// If not, the transaction could always retry, because it should be auto committed transaction.
+// Anyway the retry limit is 0, the transaction could not retry.
+func isOptimisticTxnRetryable(sessVars *variable.SessionVars, tp *sessiontxn.EnterNewTxnType) bool {


ditto A pointer is not necessary.

lcwangchao · 2022-06-28T09:31:15Z

sessiontxn/staleread/provider.go

 	default:
 		return errors.Errorf("Unsupported type: %v", tp)
 	}
+}
+
+func (p *StalenessTxnContextProvider) enterNewStaleTxn(ctx context.Context) error {


I think it's name can be activateTxn or activateStaleTxn or just merge the codes to ActivateTxn

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

lcwangchao

Rest LGTM

sessiontxn/isolation/optimistic.go

Co-authored-by: 王超 <cclcwangchao@hotmail.com>

you06 · 2022-06-29T06:53:57Z

sessiontxn/staleread/provider.go

@@ -27,9 +28,11 @@ import (

 // StalenessTxnContextProvider implements sessiontxn.TxnContextProvider
 type StalenessTxnContextProvider struct {
+	ctx  context.Context


It's a bit weird to store the go context in a struct and update it in other methods that accept the latest context(I also find the same usage in other places), the go context may be modified before the next usage like ActivateTxn, but we still use the old one. Will making the function signature like ActivateTxn(context.Context) (kv.Transaction, error) better?

ctx will be updated in provider when OnInitialize/OnStmtStart/OnStmtRetry is called which guarantees that the ctx is the same with the current statment context. On the other hand, if we choose to convey ctx in parameter, we will make many modifications as GetStmtReadTS/GetStmtForUpdateTS... all need to add this parameter.

you06 · 2022-06-29T07:04:30Z

sessiontxn/staleread/provider.go

+		if err != nil {
+			return err
+		}
+		p.is = temptable.AttachLocalTemporaryTableInfoSchema(p.sctx, is)


Yes, I think so.

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

SpadeA-Tang · 2022-06-29T09:27:16Z

/run-mysql-test

SpadeA-Tang · 2022-06-29T14:13:52Z

/run-mysql-test

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

xhebox · 2022-06-30T05:19:56Z

/merge

ti-chi-bot · 2022-06-30T05:20:00Z

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

Commit hash: 8789554

sre-bot · 2022-06-30T06:16:27Z

TiDB MergeCI notify

🔴 Bad News! [1] CI still failing after this pr merged.
These failed integration tests don't seem to be introduced by the current PR.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test	🔴 failed 1, success 10, total 11	26 min	Existing failure
idc-jenkins-ci/integration-cdc-test	🟢 all 35 tests passed	30 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 12 tests passed	15 min	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	7 min 3 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	6 min 38 sec	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	6 min 11 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 44 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	2 min 50 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 39 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

SpadeA-Tang added 4 commits June 22, 2022 18:01

init

233a8df

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

init

b51a332

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

Merge branch 'master' into txnCallManager

3b91cbc

update

7106400

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot added do-not-merge/needs-linked-issue do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 23, 2022

lcwangchao reviewed Jun 23, 2022

View reviewed changes

update

c4786a9

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot removed the do-not-merge/needs-linked-issue label Jun 24, 2022

SpadeA-Tang and others added 2 commits June 24, 2022 15:00

Merge branch 'master' into txnCallManager

3bd122d

update

19b00f6

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 27, 2022

SpadeA-Tang added 2 commits June 27, 2022 19:30

update

6241b73

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

fmt

3b8c815

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

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

Merge branch 'master' into txnCallManager

6df2d89

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

update

f6d84cd

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

update

7edae7a

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

SpadeA-Tang added the sig/transaction SIG:Transaction label Jun 28, 2022

lcwangchao reviewed Jun 28, 2022

View reviewed changes

SpadeA-Tang and others added 5 commits June 28, 2022 15:53

update

6f0d179

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

update

8e1bb57

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

Merge branch 'master' into txnCallManager

107751f

add test

288c66b

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

Merge branch 'txnCallManager' of github.com:SpadeA-Tang/tidb into txn…

6e87d6d

…CallManager

lcwangchao reviewed Jun 28, 2022

View reviewed changes

SpadeA-Tang added 4 commits June 28, 2022 17:49

update

e102e6c

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

update

769c4b4

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

add comment

3f84aee

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

update comment

3e595df

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

SpadeA-Tang added the release-note-none Denotes a PR that doesn't merit a release note. label Jun 29, 2022

ti-chi-bot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jun 29, 2022

lcwangchao reviewed Jun 29, 2022

View reviewed changes

sessiontxn/isolation/optimistic.go Outdated Show resolved Hide resolved

Update sessiontxn/isolation/optimistic.go

cfe4781

Co-authored-by: 王超 <cclcwangchao@hotmail.com>

lcwangchao approved these changes Jun 29, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 29, 2022

you06 reviewed Jun 29, 2022

View reviewed changes

update

9ebe374

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

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

SpadeA-Tang added 2 commits June 30, 2022 09:30

update

a71583f

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

resolve merge

43f7e7b

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

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

update

8789554

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

you06 approved these changes Jun 30, 2022

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 Jun 30, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 30, 2022

Merge branch 'master' into txnCallManager

8ffb6eb

ti-chi-bot merged commit 11f39cd into pingcap:master Jun 30, 2022

SpadeA-Tang mentioned this pull request Jul 1, 2022

select * from information_schema.ddl_jobs as of timestamp ... failed #35686

Closed

txn: unify the management of transaction activation by TxnManager. #35679

txn: unify the management of transaction activation by TxnManager. #35679

Conversation

SpadeA-Tang commented Jun 23, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

ti-chi-bot commented Jun 23, 2022 • 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

Choose a reason for hiding this comment

sre-bot commented Jun 27, 2022 • 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

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

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

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

lcwangchao 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

SpadeA-Tang commented Jun 29, 2022

SpadeA-Tang commented Jun 29, 2022

xhebox commented Jun 30, 2022

ti-chi-bot commented Jun 30, 2022

sre-bot commented Jun 30, 2022

TiDB MergeCI notify

SpadeA-Tang commented Jun 23, 2022 •

edited

Loading

ti-chi-bot commented Jun 23, 2022 •

edited

Loading

sre-bot commented Jun 27, 2022 •

edited

Loading