*: isolate more variables for `runWithSystemSession` #54791

lance6716 · 2024-07-22T06:43:01Z

What problem does this PR solve?

Issue Number: close #54343

Problem Summary:

What changed and how does it work?

as title

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.

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

tiprow · 2024-07-22T06:43:21Z

Hi @lance6716. 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-sigs/prow repository.

D3Hunter · 2024-07-22T07:04:19Z

pkg/executor/executor.go

@@ -1466,6 +1466,11 @@ func init() {
 			return nil, err
 		}

+		tm := sessiontxn.GetTxnManager(sctx)
+		err = tm.EnterNewTxn(ctx, &sessiontxn.EnterNewTxnRequest{})


should we commit or rollback before return?

codecov · 2024-07-22T07:05:13Z

Codecov Report

Attention: Patch coverage is 40.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 56.4529%. Comparing base (07f7774) to head (012e6bb).
Report is 741 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #54791         +/-   ##
=================================================
- Coverage   74.7413%   56.4529%   -18.2884%     
=================================================
  Files          1555       1680        +125     
  Lines        363372     616377     +253005     
=================================================
+ Hits         271589     347963      +76374     
- Misses        72138     244974     +172836     
- Partials      19645      23440       +3795

Flag	Coverage Δ
integration	`37.9157% <40.0000%> (?)`
unit	`72.1719% <40.0000%> (-1.4941%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9656% <ø> (-2.2339%)`	⬇️
parser	`∅ <ø> (∅)`
br	`52.3833% <ø> (+4.7078%)`	⬆️

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

lance6716 · 2024-07-23T03:16:53Z

/retest

tiprow · 2024-07-23T03:17:15Z

@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

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

lance6716 · 2024-07-23T03:51:24Z

/check-issue-triage-complete

D3Hunter · 2024-07-23T05:43:38Z

pkg/executor/infoschema_reader.go

@@ -898,6 +897,7 @@ func (e *hugeMemTableRetriever) dataForColumnsInTable(ctx context.Context, sctx
 			internalCtx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnOthers)
 			// Build plan is not thread safe, there will be concurrency on sessionctx.
 			if err := runWithSystemSession(internalCtx, sctx, func(s sessionctx.Context) error {
+				is := sessiontxn.GetTxnManager(s).GetTxnInfoSchema()


is this where the fix is, replace with another session ctx?

The original error is "ctx provider not found", but it's overwritten #54809. So the first fix is add sessiontxn.NewTxn.

Then another problem is the system session still uses the is captured by closure, which has conflict with user session. So moving it here.

D3Hunter · 2024-07-23T05:44:20Z

pkg/executor/show.go

@@ -2377,5 +2378,12 @@ func runWithSystemSession(ctx context.Context, sctx sessionctx.Context, fn func(
 		return err
 	}
 	defer b.ReleaseSysSession(ctx, sysCtx)
+	if err = sessiontxn.NewTxn(ctx, sysCtx); err != nil {


please add some comment on why we must open an explicit txn

lance6716 · 2024-07-23T05:45:48Z

/hold

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

lance6716 · 2024-07-23T06:07:05Z

/unhold

ti-chi-bot · 2024-07-23T06:15:11Z

[LGTM Timeline notifier]

Timeline:

2024-07-23 04:25:37.773574984 +0000 UTC m=+932759.764516455: ☑️ agreed by tangenta.
2024-07-23 06:15:10.350758657 +0000 UTC m=+939332.341700126: ☑️ agreed by D3Hunter.

lance6716 · 2024-07-23T09:01:15Z

/retest

tiprow · 2024-07-23T09:01:36Z

@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

lance6716 · 2024-07-23T09:18:56Z

/retest

tiprow · 2024-07-23T09:19:17Z

@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

ti-chi-bot · 2024-07-23T10:36:47Z

In response to a cherrypick label: new pull request created to branch release-7.5: #54843.

Signed-off-by: ti-chi-bot <[email protected]>

close #54343

ti-chi-bot · 2024-07-26T07:43:14Z

In response to a cherrypick label: new pull request created to branch release-6.5: #54937.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-07-26T07:45:52Z

In response to a cherrypick label: new pull request created to branch release-8.1: #54949.

close #54343

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-10-28T08:21:43Z

In response to a cherrypick label: new pull request created to branch release-7.1: #56902.

close #54343

*: EnterNewTxn before EvalSubqueryFirstRow

d175a4a

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

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 22, 2024

ti-chi-bot added the needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. label Jul 22, 2024

ti-chi-bot bot added sig/planner SIG: Planner size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 22, 2024

D3Hunter reviewed Jul 22, 2024

View reviewed changes

lance6716 added 2 commits July 22, 2024 16:19

address comment

b49db20

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

fix UT

6b4440f

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

fix UT

e614326

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

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Jul 23, 2024

lance6716 changed the title *: EnterNewTxn before EvalSubqueryFirstRow *: isolated more variables for runWithSystemSession Jul 23, 2024

lance6716 changed the title *: isolated more variables for runWithSystemSession *: isolate more variables for runWithSystemSession Jul 23, 2024

tangenta approved these changes Jul 23, 2024

View reviewed changes

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

D3Hunter reviewed Jul 23, 2024

View reviewed changes

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2024

add comment

012e6bb

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

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2024

D3Hunter approved these changes Jul 23, 2024

View reviewed changes

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

ti-chi-bot bot merged commit b19a918 into pingcap:master Jul 23, 2024
23 checks passed

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jul 23, 2024

This is an automated cherry-pick of pingcap#54791

8d27995

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Jul 23, 2024

*: isolate more variables for runWithSystemSession (#54791) #54843

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Jul 24, 2024

*: isolate more variables for runWithSystemSession (#54791) (#54843)

406ffe9

close #54343

ti-chi-bot added the needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. label Jul 26, 2024

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jul 26, 2024

This is an automated cherry-pick of pingcap#54791

1bbc0e9

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Jul 26, 2024

*: isolate more variables for runWithSystemSession (#54791) #54937

Merged

13 tasks

ti-chi-bot added the needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. label Jul 26, 2024

ti-chi-bot mentioned this pull request Jul 26, 2024

*: isolate more variables for runWithSystemSession (#54791) #54949

Merged

13 tasks

This was referenced Jul 29, 2024

executor: runWithSystemSession also copy snapshot status (#54989) #55028

Merged

executor: runWithSystemSession also copy snapshot status (#54989) #55048

Merged

ti-chi-bot bot pushed a commit that referenced this pull request Jul 30, 2024

*: isolate more variables for runWithSystemSession (#54791) (#54949)

7a8023e

close #54343

ti-chi-bot mentioned this pull request Aug 26, 2024

executor: runWithSystemSession also copy snapshot status (#54989) #55644

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Aug 26, 2024

*: isolate more variables for runWithSystemSession (#54791) (#54937)

e17bd5b

close #54343

ti-chi-bot bot added the needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. label Oct 28, 2024

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Oct 28, 2024

This is an automated cherry-pick of pingcap#54791

cf9c5aa

Signed-off-by: ti-chi-bot <[email protected]>

This was referenced Oct 28, 2024

*: isolate more variables for runWithSystemSession (#54791) #56902

Merged

executor: runWithSystemSession also copy snapshot status (#54989) #56907

Merged

ti-chi-bot bot pushed a commit that referenced this pull request Nov 6, 2024

*: isolate more variables for runWithSystemSession (#54791) (#56902)

2af9e79

close #54343

lance6716 mentioned this pull request Dec 12, 2024

INFORMATION_SCHEMA.columns performance regression after fix #54343 #58184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: isolate more variables for `runWithSystemSession` #54791

*: isolate more variables for `runWithSystemSession` #54791

lance6716 commented Jul 22, 2024

tiprow bot commented Jul 22, 2024

D3Hunter Jul 22, 2024

codecov bot commented Jul 22, 2024 •

edited

Loading

lance6716 commented Jul 23, 2024

tiprow bot commented Jul 23, 2024

lance6716 commented Jul 23, 2024

D3Hunter Jul 23, 2024

lance6716 Jul 23, 2024

D3Hunter Jul 23, 2024

lance6716 commented Jul 23, 2024

lance6716 commented Jul 23, 2024

ti-chi-bot bot commented Jul 23, 2024

lance6716 commented Jul 23, 2024

tiprow bot commented Jul 23, 2024

lance6716 commented Jul 23, 2024

tiprow bot commented Jul 23, 2024

ti-chi-bot commented Jul 23, 2024

ti-chi-bot commented Jul 26, 2024

ti-chi-bot commented Jul 26, 2024

ti-chi-bot commented Oct 28, 2024

*: isolate more variables for runWithSystemSession #54791

*: isolate more variables for runWithSystemSession #54791

Conversation

lance6716 commented Jul 22, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jul 22, 2024

D3Hunter Jul 22, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 22, 2024 • edited Loading

Codecov Report

lance6716 commented Jul 23, 2024

tiprow bot commented Jul 23, 2024

lance6716 commented Jul 23, 2024

D3Hunter Jul 23, 2024

Choose a reason for hiding this comment

lance6716 Jul 23, 2024

Choose a reason for hiding this comment

D3Hunter Jul 23, 2024

Choose a reason for hiding this comment

lance6716 commented Jul 23, 2024

lance6716 commented Jul 23, 2024

ti-chi-bot bot commented Jul 23, 2024

[LGTM Timeline notifier]

lance6716 commented Jul 23, 2024

tiprow bot commented Jul 23, 2024

lance6716 commented Jul 23, 2024

tiprow bot commented Jul 23, 2024

ti-chi-bot commented Jul 23, 2024

ti-chi-bot commented Jul 26, 2024

ti-chi-bot commented Jul 26, 2024

ti-chi-bot commented Oct 28, 2024

*: isolate more variables for `runWithSystemSession` #54791

*: isolate more variables for `runWithSystemSession` #54791

codecov bot commented Jul 22, 2024 •

edited

Loading