txn: remove NewTxn and NewStaleTxnWithStartTS in session #35885

SpadeA-Tang · 2022-07-01T10:16:07Z

Signed-off-by: SpadeA-Tang [email protected]

What problem does this PR solve?

Issue Number: close #35884

Problem Summary:

What is changed and how it works?

Remove the NewTxn and NewStaleTxnWithStartTS in session and let the transaction context provider to make the creation of new transaction.

The mode of creation a new transaction now is: prepareTxn/prepareTxnWithOracleTS + activation.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: SpadeA-Tang <[email protected]>

ti-chi-bot · 2022-07-01T10:16:10Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

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

Signed-off-by: SpadeA-Tang <[email protected]>

ti-chi-bot · 2022-07-01T21:31:32Z

@SpadeA-Tang: PR needs rebase.

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 kubernetes/test-infra repository.

Signed-off-by: SpadeA-Tang <[email protected]>

sre-bot · 2022-07-04T08:30:41Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/11c1d2c9468824b1fd3f7eca4682535676475e8e

Signed-off-by: SpadeA-Tang <[email protected]>

session/session.go

Signed-off-by: SpadeA-Tang <[email protected]>

lcwangchao · 2022-07-06T11:21:09Z

session/session.go

 func (s *session) GetPreparedTxnFuture() sessionctx.TxnFuture {
-	if !s.txn.validOrPending() {


Why remove this? I think it is reasonable if a txn is not prepared the TxnFuture should return a nil value.

lcwangchao · 2022-07-06T11:22:39Z

sessiontxn/isolation/base.go

+		if err := sessiontxn.CheckBeforeNewTxn(p.ctx, p.sctx); err != nil {
+			return err
+		}
+		if err := p.prepareTxnNotConsiderSnapshotTS(); err != nil {


Is it ok if we call p.prepareTxn(false) here ?

lcwangchao · 2022-07-06T11:25:14Z

sessiontxn/staleread/provider.go

-	txnCtx.InfoSchema = p.is
-	return nil
+	txnFuture := p.sctx.GetPreparedTxnFuture()
+	if txnFuture == nil {


ts is prepared in line 97. I think we can guarantee txnFuture is not nil here. Maybe this line is not necessary

Yes, it's unnecessary.

Signed-off-by: SpadeA-Tang <[email protected]>

lcwangchao · 2022-07-07T08:02:35Z

util/mock/context.go

@@ -61,9 +61,16 @@ type Context struct {

 type wrapTxn struct {
 	kv.Transaction
+	prepared bool


Can we store the oracle.Future here instead prepared such that we can get a "real" transaction when call Wait.

sessiontxn/isolation/readcommitted.go

sessiontxn/isolation/base.go

sessiontxn/txn.go

Signed-off-by: SpadeA-Tang <[email protected]>

lcwangchao · 2022-07-07T09:04:59Z

/merge

ti-chi-bot · 2022-07-07T09:05:03Z

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

Commit hash: fd9fc8a

Signed-off-by: SpadeA-Tang <[email protected]>

SpadeA-Tang · 2022-07-07T10:26:45Z

/run-mysql-test

lcwangchao · 2022-07-07T11:05:48Z

/merge

ti-chi-bot · 2022-07-07T11:05:52Z

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

Commit hash: 7fea97c

sre-bot · 2022-07-07T12:02:51Z

TiDB MergeCI notify

🔴 Bad News! New failing [1] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci/integration-cdc-test	🟥 failed 2, success 33, total 35	30 min	New failing
idc-jenkins-ci-tidb/integration-common-test	🟢 all 11 tests passed	23 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 12 tests passed	13 min	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	6 min 37 sec	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	6 min 22 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	5 min 18 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 9 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	4 min 37 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 57 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

remove newtxn

2df07d2

Signed-off-by: SpadeA-Tang <[email protected]>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 1, 2022

SpadeA-Tang added 2 commits July 1, 2022 18:46

remove newtxnwithstalets

71775f0

Signed-off-by: SpadeA-Tang <[email protected]>

update

f128c89

Signed-off-by: SpadeA-Tang <[email protected]>

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 Jul 1, 2022

Merge branch 'master' into remoteNewTxn

0a64c39

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 1, 2022

SpadeA-Tang added 3 commits July 4, 2022 10:45

fmt

c7ab65c

Signed-off-by: SpadeA-Tang <[email protected]>

update

3de64fa

Signed-off-by: SpadeA-Tang <[email protected]>

update

4036b05

Signed-off-by: SpadeA-Tang <[email protected]>

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

update

8bdff90

Signed-off-by: SpadeA-Tang <[email protected]>

update

3c9c1ec

Signed-off-by: SpadeA-Tang <[email protected]>

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 Jul 4, 2022

xhebox reviewed Jul 4, 2022

View reviewed changes

session/session.go Outdated Show resolved Hide resolved

remove unnecessary comment

77d942c

Signed-off-by: SpadeA-Tang <[email protected]>

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

xhebox approved these changes Jul 4, 2022

View reviewed changes

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

SpadeA-Tang and others added 2 commits July 5, 2022 15:25

Merge branch 'master' into remoteNewTxn

5779eee

update comments

a7186f2

Signed-off-by: SpadeA-Tang <[email protected]>

SpadeA-Tang added 2 commits July 6, 2022 15:22

Merge branch 'master' into remoteNewTxn

8304069

Merge branch 'master' into remoteNewTxn

b0c6f15

lcwangchao reviewed Jul 6, 2022

View reviewed changes

SpadeA-Tang added 3 commits July 7, 2022 12:30

update

a3c6a56

Signed-off-by: SpadeA-Tang <[email protected]>

update

cd26fd4

Signed-off-by: SpadeA-Tang <[email protected]>

update

59fa3bd

Signed-off-by: SpadeA-Tang <[email protected]>

lcwangchao reviewed Jul 7, 2022

View reviewed changes

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

lcwangchao reviewed Jul 7, 2022

View reviewed changes

sessiontxn/txn.go Outdated Show resolved Hide resolved

update

98ffff8

Signed-off-by: SpadeA-Tang <[email protected]>

lcwangchao approved these changes Jul 7, 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 Jul 7, 2022

Merge branch 'master' into remoteNewTxn

fd9fc8a

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

ti-chi-bot and others added 2 commits July 7, 2022 17:57

Merge branch 'master' into remoteNewTxn

0a41a9c

update

7fea97c

Signed-off-by: SpadeA-Tang <[email protected]>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Jul 7, 2022

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

Merge branch 'master' into remoteNewTxn

11c1d2c

ti-chi-bot merged commit f3eb0f5 into pingcap:master Jul 7, 2022

ekexium mentioned this pull request Jul 19, 2022

Refactoring of sessiontxn introduces extra TSO waits #36311

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txn: remove NewTxn and NewStaleTxnWithStartTS in session #35885

txn: remove NewTxn and NewStaleTxnWithStartTS in session #35885

SpadeA-Tang commented Jul 1, 2022 •

edited

Loading

ti-chi-bot commented Jul 1, 2022 •

edited

Loading

ti-chi-bot commented Jul 1, 2022

sre-bot commented Jul 4, 2022 •

edited

Loading

lcwangchao Jul 6, 2022

lcwangchao Jul 6, 2022

lcwangchao Jul 6, 2022

SpadeA-Tang Jul 7, 2022

lcwangchao Jul 7, 2022

lcwangchao commented Jul 7, 2022

ti-chi-bot commented Jul 7, 2022

SpadeA-Tang commented Jul 7, 2022

lcwangchao commented Jul 7, 2022

ti-chi-bot commented Jul 7, 2022

sre-bot commented Jul 7, 2022

		func (s *session) GetPreparedTxnFuture() sessionctx.TxnFuture {
		if !s.txn.validOrPending() {

txn: remove NewTxn and NewStaleTxnWithStartTS in session #35885

txn: remove NewTxn and NewStaleTxnWithStartTS in session #35885

Conversation

SpadeA-Tang commented Jul 1, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jul 1, 2022 • edited Loading

ti-chi-bot commented Jul 1, 2022

sre-bot commented Jul 4, 2022 • edited Loading

lcwangchao Jul 6, 2022

Choose a reason for hiding this comment

lcwangchao Jul 6, 2022

Choose a reason for hiding this comment

lcwangchao Jul 6, 2022

Choose a reason for hiding this comment

SpadeA-Tang Jul 7, 2022

Choose a reason for hiding this comment

lcwangchao Jul 7, 2022

Choose a reason for hiding this comment

lcwangchao commented Jul 7, 2022

ti-chi-bot commented Jul 7, 2022

SpadeA-Tang commented Jul 7, 2022

lcwangchao commented Jul 7, 2022

ti-chi-bot commented Jul 7, 2022

sre-bot commented Jul 7, 2022

TiDB MergeCI notify

SpadeA-Tang commented Jul 1, 2022 •

edited

Loading

ti-chi-bot commented Jul 1, 2022 •

edited

Loading

sre-bot commented Jul 4, 2022 •

edited

Loading