domain,session: Store and delete internal session in the session pool by TonsnakeLin · Pull Request #34168 · pingcap/tidb

TonsnakeLin · 2022-04-22T06:39:22Z

Signed-off-by: TonsnakeLin lpbgytong@163.com

What problem does this PR solve?

Issue Number: close #34154

Problem Summary:

What is changed and how it works?

We stored an internal session to the SessionManger and delete it in the function (s *session) getInternalSession, but not all internal sessions were got from it. We now store an internal session to the SessionManger and delete it from SessionPool's get and 'put' methods, so no internal sessions are lost.

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: TonsnakeLin <lpbgytong@163.com>

ti-chi-bot · 2022-04-22T06:39:24Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

cfzjywxk
sticnarf

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.

Details

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

sre-bot · 2022-04-22T06:50:11Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/7749ddc1bfe2c87bc2544f20ab34a26dac50ef82

TonsnakeLin · 2022-04-22T06:59:48Z

cc @cfzjywxk @sticnarf @youjiali1995 @MyonKeminta

sticnarf · 2022-04-22T07:04:56Z

Is it possible to cover it with a unit test?

TonsnakeLin · 2022-04-22T08:12:50Z

Is it possible to cover it with a unit test?

I'll try it

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

TonsnakeLin · 2022-04-27T12:40:45Z

@cfzjywxk @sticnarf @youjiali1995 @MyonKeminta

cfzjywxk · 2022-04-28T02:34:06Z

session/session.go

 		startTS = txnInfo.StartTS
 	}
+
+	logutil.BgLogger().Info(


This could be changed to DEBUG level.

cfzjywxk · 2022-04-28T02:35:19Z

domain/db_test.go

 }
+
+func TestNormalSessionPool(t *testing.T) {
+	lease := 50 * time.Millisecond


Increasing the lease maybe could prevent some weird failures.

sticnarf

I have only one small question.

What if some error happens after getInternalSession gets a session from the pool? I find getSnapshotInfoSchema may raise an error.

It seems like a resource leak. But it seems to me that it does not affect calculating the min start TS, right?

TonsnakeLin · 2022-04-28T06:15:26Z

Yes, I think you find a bug when use getInternalSession to get internal sessions from session pool. It does not affect calculating the min start TS, because the startTS of the session leaked is always 0.

I have only one small question.

What if some error happens after getInternalSession gets a session from the pool? I find getSnapshotInfoSchema may raise an error.

It seems like a resource leak. But it seems to me that it does not affect calculating the min start TS, right?

sticnarf

I don't have more comments except those from @cfzjywxk

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

sticnarf · 2022-04-28T06:25:47Z

Yes, I think you find a bug when use getInternalSession to get internal sessions from session pool. It does not affect calculating the min start TS, because the startTS of the session leaked is always 0.

Would you mind giving a quick fix here in this PR? It might not be a real problem before, because the unused session should be GCed by the runtime, but here it is a real leak because it's saved to the map now.

cfzjywxk · 2022-04-29T01:43:41Z

/merge

ti-chi-bot · 2022-04-29T01:43:44Z

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

Details

Commit hash: 5678cc9

TonsnakeLin · 2022-04-29T02:11:07Z

/run-mysql-test

TonsnakeLin · 2022-04-29T02:11:27Z

/run-unit-test

TonsnakeLin · 2022-04-29T02:40:36Z

/run-mysql-test

sre-bot · 2022-04-29T06:33:59Z

TiDB MergeCI notify

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci/integration-cdc-test	🟢 all 34 tests passed	32 min	Existing passed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 11 tests passed	17 min	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	16 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 12 tests passed	12 min	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	10 min	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	9 min 1 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	8 min 53 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	5 min 43 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	4 min 57 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

…pingcap#34168) close pingcap#34154

TonsnakeLin · 2022-05-04T12:00:12Z

but here it is a real leak because it's saved to the map no

I'll fix it later

close pingcap#34154

b72b365

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

ti-chi-bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 22, 2022

ti-chi-bot removed the do-not-merge/needs-triage-completed label Apr 22, 2022

hawkingrei requested a review from lcwangchao April 23, 2022 08:04

add unit test

a31c280

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 27, 2022

cfzjywxk reviewed Apr 28, 2022

View reviewed changes

sticnarf reviewed Apr 28, 2022

View reviewed changes

sticnarf approved these changes Apr 28, 2022

View reviewed changes

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

TonsnakeLin added 2 commits April 28, 2022 14:20

change log level and fix testcase

1dd694c

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

fix testcase

5678cc9

Signed-off-by: TonsnakeLin <lpbgytong@163.com>

TonsnakeLin requested a review from cfzjywxk April 28, 2022 06:23

cfzjywxk approved these changes Apr 29, 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 Apr 29, 2022

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

Merge branch 'master' into dev-debug-innerTxn

05d498d

Merge branch 'master' into dev-debug-innerTxn

7749ddc

ti-chi-bot merged commit 5b6ffa7 into pingcap:master Apr 29, 2022

espresso98 pushed a commit to espresso98/tidb that referenced this pull request Apr 29, 2022

domain,session: Store and delete internal session in the session pool (…

ab6606c

…pingcap#34168) close pingcap#34154

Conversation

TonsnakeLin commented Apr 22, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sre-bot commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TonsnakeLin commented Apr 22, 2022

Uh oh!

sticnarf commented Apr 22, 2022

Uh oh!

TonsnakeLin commented Apr 22, 2022

Uh oh!

TonsnakeLin commented Apr 27, 2022

Uh oh!

cfzjywxk Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

TonsnakeLin Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

cfzjywxk Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

TonsnakeLin Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

sticnarf left a comment

Choose a reason for hiding this comment

Uh oh!

TonsnakeLin commented Apr 28, 2022

Uh oh!

sticnarf left a comment

Choose a reason for hiding this comment

Uh oh!

sticnarf commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfzjywxk commented Apr 29, 2022

Uh oh!

ti-chi-bot commented Apr 29, 2022

Uh oh!

TonsnakeLin commented Apr 29, 2022

Uh oh!

TonsnakeLin commented Apr 29, 2022

Uh oh!

TonsnakeLin commented Apr 29, 2022

Uh oh!

sre-bot commented Apr 29, 2022

TiDB MergeCI notify

Uh oh!

TonsnakeLin commented May 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ti-chi-bot commented Apr 22, 2022 •

edited

Loading

sre-bot commented Apr 22, 2022 •

edited

Loading

sticnarf commented Apr 28, 2022 •

edited

Loading