txn: set txn options in txn provider which avoid data race #52304

you06 · 2024-04-02T07:24:31Z

What problem does this PR solve?

Issue Number: ref #50215

Problem Summary:

Move most SetOption usages into txn provider, manage them together, this can help us avoid data race in clear way.

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

The test in sessiontxn package is passed with this PR's modification.

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: you06 <[email protected]>

tiprow · 2024-04-02T07:24:48Z

Hi @you06. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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: you06 <[email protected]>

codecov · 2024-04-02T07:45:11Z

Codecov Report

Merging #52304 (7df782c) into master (7a20e43) will increase coverage by 1.9470%.
Report is 30 commits behind head on master.
The diff coverage is 79.4594%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #52304        +/-   ##
================================================
+ Coverage   72.1416%   74.0886%   +1.9470%     
================================================
  Files          1467       1467                
  Lines        426665     429290      +2625     
================================================
+ Hits         307803     318055     +10252     
+ Misses        99697      91320      -8377     
- Partials      19165      19915       +750

Flag	Coverage Δ
integration	`28.4150% <71.8918%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`53.9957% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`49.5528% <ø> (+8.9886%)`	⬆️

Signed-off-by: you06 <[email protected]>

hawkingrei · 2024-04-02T09:06:42Z

/ok-to-test

Signed-off-by: you06 <[email protected]>

pkg/sessiontxn/isolation/base.go

cfzjywxk · 2024-04-03T04:19:17Z

pkg/sessiontxn/isolation/base.go

+	}
+
+	if interceptor := temptable.SessionSnapshotInterceptor(p.sctx, p.infoSchema); interceptor != nil {
+		txn.SetOption(kv.SnapInterceptor, interceptor)


The txnSetOption should be deprecated to avoid mis-usages in the future.

txn.SetOption is widely used in some internal packages, I don't have confidence to deprecate it since it takes too many capabilities.

Or is it possible to also check the newly added isCommitterworking inside the tikvTxn.SetOption?

I did it so, but only in test environment, see https://github.com/pingcap/tidb/pull/52304/files#diff-2aa8e8b32ea03a38f0f9a2850ed09a21848ee98686785d3a7dcee9a4d530f8b9R217-R219

Why just check in test? I think checking the txn status when SetOption is the key to avoid future mistakes.

Signed-off-by: you06 <[email protected]>

you06 · 2024-04-03T10:56:19Z

/retest

zyguan · 2024-04-03T11:11:50Z

pkg/session/session.go


+	sessiontxn.GetTxnManager(s)


What's the purpose of this call?

Good catch, that's a mistake

zyguan · 2024-04-03T11:49:38Z

pkg/sessiontxn/isolation/base.go

+		}
+		return nil
+	}
+


Do we forget to set kv.CommitTSUpperBoundCheck when commitTSChecker is not nil?

Signed-off-by: you06 <[email protected]>

cfzjywxk · 2024-04-03T12:36:16Z

pkg/sessiontxn/isolation/base.go

+	}
+
+	if interceptor := temptable.SessionSnapshotInterceptor(p.sctx, p.infoSchema); interceptor != nil {
+		txn.SetOption(kv.SnapInterceptor, interceptor)


Why just check in test? I think checking the txn status when SetOption is the key to avoid future mistakes.

cfzjywxk · 2024-04-03T12:38:20Z

pkg/store/driver/txn/txn_driver.go

@@ -200,6 +214,9 @@ func (txn *tikvTxn) GetMemBuffer() kv.MemBuffer {
 }

 func (txn *tikvTxn) SetOption(opt int, val any) {
+	if intest.InTest {


Why just check in tests?

I suppose the current implementation is ok, if this check only works in tests, we can just panic rather than returning and checking error. Another reason, compile with intest.InTest won't impact the performance of release builds.

cfzjywxk · 2024-04-03T12:40:33Z

pkg/store/driver/txn/txn_driver.go

@@ -78,6 +81,10 @@ func (txn *tikvTxn) CacheTableInfo(id int64, info *model.TableInfo) {
 }

 func (txn *tikvTxn) LockKeys(ctx context.Context, lockCtx *kv.LockCtx, keysInput ...kv.Key) error {
+	if intest.InTest {
+		txn.isCommitterWorking.Store(true)


Why set isCommitterWorking when InTest? The InTest is a little confusing.

ti-chi-bot · 2024-04-04T09:07:09Z

[LGTM Timeline notifier]

Timeline:

2024-04-04 01:55:46.962630041 +0000 UTC m=+495408.490170585: ☑️ agreed by cfzjywxk.
2024-04-04 09:07:08.923274114 +0000 UTC m=+521290.450814660: ☑️ agreed by zyguan.

cfzjywxk · 2024-04-06T04:05:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cfzjywxk, zyguan Once this PR has been reviewed and has the lgtm label, please assign tangenta, yudongusa for approval, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfzjywxk,zyguan]

pkg/ddl/OWNERS

pkg/sessionctx/variable/OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

@you06 Need approvements from the owners.

cfzjywxk · 2024-04-07T01:57:25Z

@easonn7 PTAL

cfzjywxk · 2024-04-07T01:57:49Z

@wjhuang2016 @tangenta
PTAL

easonn7 · 2024-04-07T01:59:49Z

/approve

ti-chi-bot · 2024-04-08T08:24:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, easonn7, zimulala, zyguan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfzjywxk,zimulala,zyguan]
~~pkg/ddl/OWNERS~~ [zimulala]
~~pkg/sessionctx/variable/OWNERS~~ [easonn7]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

you06 · 2024-04-08T08:28:42Z

/merge

ti-chi-bot · 2024-04-08T08:28:43Z

@you06: We have migrated to builtin LGTM and approve plugins for reviewing.

Please use /approve when you want approve this pull request.

The changes announcement: LGTM plugin changes

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.

set options in txn provider

dc22ece

Signed-off-by: you06 <[email protected]>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 2, 2024

move Set/Get/Clear DiskFullOpt into session variables

6f0b47b

Signed-off-by: you06 <[email protected]>

you06 added 2 commits April 2, 2024 16:49

bazel

a9322d7

Signed-off-by: you06 <[email protected]>

set linearizability for EnterNewTxnDefault

6fbc56c

Signed-off-by: you06 <[email protected]>

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 2, 2024

you06 added 2 commits April 2, 2024 20:38

set resource tagger for ddl

d74436e

Signed-off-by: you06 <[email protected]>

Merge branch 'master' into race/p-dml-set-option

466dccc

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

fix grammar lint

24e1573

Signed-off-by: you06 <[email protected]>

cfzjywxk requested review from cfzjywxk, zyguan, MyonKeminta, crazycs520 and ekexium April 3, 2024 04:06

cfzjywxk reviewed Apr 3, 2024

View reviewed changes

you06 added 3 commits April 3, 2024 15:50

check if committer working intest

880a846

Signed-off-by: you06 <[email protected]>

check inside tikv driver

07de0f4

Signed-off-by: you06 <[email protected]>

fix mis check

538fdad

Signed-off-by: you06 <[email protected]>

zyguan reviewed Apr 3, 2024

View reviewed changes

address comment

7df782c

Signed-off-by: you06 <[email protected]>

cfzjywxk reviewed Apr 3, 2024

View reviewed changes

cfzjywxk requested a review from zyguan April 4, 2024 01:55

cfzjywxk approved these changes Apr 4, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Apr 4, 2024

zyguan approved these changes Apr 4, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 4, 2024

zimulala approved these changes Apr 8, 2024

View reviewed changes

ti-chi-bot bot added the approved label Apr 8, 2024

ti-chi-bot bot merged commit c39d79f into pingcap:master Apr 8, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txn: set txn options in txn provider which avoid data race #52304

txn: set txn options in txn provider which avoid data race #52304

you06 commented Apr 2, 2024

tiprow bot commented Apr 2, 2024

codecov bot commented Apr 2, 2024 •

edited

Loading

hawkingrei commented Apr 2, 2024

cfzjywxk Apr 3, 2024

you06 Apr 3, 2024

cfzjywxk Apr 3, 2024

you06 Apr 3, 2024

cfzjywxk Apr 3, 2024

you06 commented Apr 3, 2024

zyguan Apr 3, 2024

you06 Apr 3, 2024

zyguan Apr 3, 2024

you06 Apr 3, 2024

cfzjywxk Apr 3, 2024

cfzjywxk Apr 3, 2024

you06 Apr 3, 2024

cfzjywxk Apr 3, 2024

ti-chi-bot bot commented Apr 4, 2024

cfzjywxk commented Apr 6, 2024

cfzjywxk commented Apr 7, 2024

cfzjywxk commented Apr 7, 2024 •

edited

Loading

easonn7 commented Apr 7, 2024

ti-chi-bot bot commented Apr 8, 2024

you06 commented Apr 8, 2024

ti-chi-bot bot commented Apr 8, 2024

txn: set txn options in txn provider which avoid data race #52304

txn: set txn options in txn provider which avoid data race #52304

Conversation

you06 commented Apr 2, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Apr 2, 2024

codecov bot commented Apr 2, 2024 • edited Loading

Codecov Report

hawkingrei commented Apr 2, 2024

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

you06 commented Apr 3, 2024

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

ti-chi-bot bot commented Apr 4, 2024

[LGTM Timeline notifier]

cfzjywxk commented Apr 6, 2024

cfzjywxk commented Apr 7, 2024

cfzjywxk commented Apr 7, 2024 • edited Loading

easonn7 commented Apr 7, 2024

ti-chi-bot bot commented Apr 8, 2024

you06 commented Apr 8, 2024

ti-chi-bot bot commented Apr 8, 2024

codecov bot commented Apr 2, 2024 •

edited

Loading

cfzjywxk commented Apr 7, 2024 •

edited

Loading