*: global memory controller should not kill session whose mem less than limit_sess_min_size #42803

XuHuaiyu · 2023-04-04T06:57:35Z

What problem does this PR solve?

Issue Number:
close #42662
close #42664

Problem Summary:
As the description mentioned here:
#42662 (comment)

What is changed and how it works?

Reset NeekKill flag even error is thrown during the planner phase or executor.Open phase
Do not kill the session whose memory usage is less than tidb_server_memory_limit_sess_min_size

Check List

Tests

Unit test
Integration test

If we run the same test on master branch, the following error will be thrown:

    testkit.go:157: 
        	Error Trace:	/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/testkit/testkit.go:157
        	            				/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/testkit/testkit.go:150
        	            				/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/executor/executor_test.go:6521
        	Error:      	Received unexpected error:
        	            	Out Of Memory Quota![conn=12345]
        	            	github.com/pingcap/tidb/executor.(*ExecStmt).openExecutor.func1
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/executor/adapter.go:1188
        	            	runtime.gopanic
        	            		/Users/xuhuaiyu/.gvm/gos/go1.20/src/runtime/panic.go:884
        	            	github.com/pingcap/tidb/util/memory.(*PanicOnExceed).Action
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/util/memory/action.go:149
        	            	github.com/pingcap/tidb/util/memory.(*Tracker).Consume.func2
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/util/memory/tracker.go:450
        	            	github.com/pingcap/tidb/util/memory.(*Tracker).Consume
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/util/memory/tracker.go:462
        	            	github.com/pingcap/tidb/executor.(*HashAggExec).initForParallelExec
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/executor/aggregate.go:395
        	            	github.com/pingcap/tidb/executor.(*HashAggExec).Open
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/executor/aggregate.go:321
        	            	github.com/pingcap/tidb/executor.(*ExecStmt).openExecutor
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/executor/adapter.go:1192
        	            	github.com/pingcap/tidb/executor.(*ExecStmt).Exec
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/executor/adapter.go:527
        	            	github.com/pingcap/tidb/session.runStmt
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/session/session.go:2343
        	            	github.com/pingcap/tidb/session.(*session).ExecuteStmt
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/session/session.go:2200
        	            	github.com/pingcap/tidb/testkit.(*TestKit).ExecWithContext
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/testkit/testkit.go:325
        	            	github.com/pingcap/tidb/testkit.(*TestKit).MustQueryWithContext
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/testkit/testkit.go:156
        	            	github.com/pingcap/tidb/testkit.(*TestKit).MustQuery
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/testkit/testkit.go:150
        	            	github.com/pingcap/tidb/executor_test.TestIssue42662
        	            		/Users/xuhuaiyu/Development/GOPATH/src/github.com/pingcap/tidb/executor/executor_test.go:6521
        	            	testing.tRunner
        	            		/Users/xuhuaiyu/.gvm/gos/go1.20/src/testing/testing.go:1576
        	            	runtime.goexit
        	            		/Users/xuhuaiyu/.gvm/gos/go1.20/src/runtime/asm_amd64.s:1598
        	Test:       	TestIssue42662
        	Messages:   	sql:select count(*) from t1, args:[]

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.

Fix the problem that global memory controller kill the sqls whose memory usage are less than tidb_server_memory_limit_sess_min_size defined.

…an limit_sess_min_size

ti-chi-bot · 2023-04-04T06:57:37Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

chrysan
hawkingrei

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.

chrysan · 2023-04-04T14:48:05Z

util/servermemorylimit/servermemorylimit.go

+			memUsage := t.BytesConsumed()
+			// If the memory usage of the top1 session is less than tidb_server_memory_limit_sess_min_size, we do not need to kill it.
+			if uint64(memUsage) < limitSessMinSize {
+				memory.MemUsageTop1Tracker.CompareAndSwap(t, nil)


If CompareAndSwap fails, it means the Top1Tracker is another tracker now, but we will skip the check and kill in this round, right?

It looks OK, I see the interval is 100 ms.

hawkingrei · 2023-04-05T11:59:33Z

executor/executor_test.go

@@ -6481,3 +6481,46 @@ from ssci right join csci on (ssci.customer_sk=csci.customer_sk
                               and ssci.item_sk = csci.item_sk)
 limit 100;`)
 }
+
+func TestIssue42662(t *testing.T) {


Because too many test cases are in the executor. Please move it into executor/issuetest.

on the other hand, TestIssuexxxx makes people wonder what exactly has been repaired.

hawkingrei · 2023-04-05T12:00:55Z

/retest

XuHuaiyu · 2023-04-06T05:16:13Z

/retest

XuHuaiyu · 2023-04-06T05:28:15Z

/retest

hawkingrei · 2023-04-06T07:47:56Z

/retest

XuHuaiyu · 2023-04-07T04:12:02Z

/merge

ti-chi-bot · 2023-04-07T04:12:06Z

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

Commit hash: ff2bef0

ti-chi-bot · 2023-04-07T05:03:41Z

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

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

…an limit_sess_min_size (#42803) (#42852)

*: global memory controller should not kill session whose mem less th…

89beebf

…an limit_sess_min_size

XuHuaiyu added type/bugfix This PR fixes a bug. sig/execution SIG execution needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. labels Apr 4, 2023

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 4, 2023

XuHuaiyu changed the title *: global memory controller should not kill session whose mem less th… *: global memory controller should not kill session whose mem less than limit_sess_min_size Apr 4, 2023

add test

c0a7728

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 Apr 4, 2023

XuHuaiyu added 2 commits April 4, 2023 17:16

fix typo

debcf22

make bazel_prepare

9a9ec67

chrysan reviewed Apr 4, 2023

View reviewed changes

chrysan approved these changes Apr 4, 2023

View reviewed changes

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

hawkingrei reviewed Apr 5, 2023

View reviewed changes

XuHuaiyu added 2 commits April 6, 2023 10:59

address comment

64feb1c

go fmt

ff2bef0

hawkingrei approved these changes Apr 6, 2023

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 6, 2023

hawkingrei requested a review from wshwsh12 April 6, 2023 06:37

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

ti-chi-bot merged commit d6a5054 into pingcap:master Apr 7, 2023

ti-chi-bot mentioned this pull request Apr 7, 2023

*: global memory controller should not kill session whose mem less than limit_sess_min_size (#42803) #42852

Merged

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Apr 7, 2023

This is an automated cherry-pick of pingcap#42803

f66bd48

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

ti-chi-bot added a commit that referenced this pull request Apr 10, 2023

*: global memory controller should not kill session whose mem less th…

0105222

…an limit_sess_min_size (#42803) (#42852)

This was referenced Apr 17, 2023

util: remove useless debug log #43089

Merged

util: remove useless debug log (#43089) #43115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: global memory controller should not kill session whose mem less than limit_sess_min_size #42803

*: global memory controller should not kill session whose mem less than limit_sess_min_size #42803

XuHuaiyu commented Apr 4, 2023 •

edited

Loading

ti-chi-bot commented Apr 4, 2023 •

edited

Loading

chrysan Apr 4, 2023

chrysan Apr 4, 2023

hawkingrei Apr 5, 2023

hawkingrei commented Apr 5, 2023

XuHuaiyu commented Apr 6, 2023

XuHuaiyu commented Apr 6, 2023

hawkingrei commented Apr 6, 2023

XuHuaiyu commented Apr 7, 2023

ti-chi-bot commented Apr 7, 2023

ti-chi-bot commented Apr 7, 2023

*: global memory controller should not kill session whose mem less than limit_sess_min_size #42803

*: global memory controller should not kill session whose mem less than limit_sess_min_size #42803

Conversation

XuHuaiyu commented Apr 4, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 4, 2023 • edited Loading

chrysan Apr 4, 2023

Choose a reason for hiding this comment

chrysan Apr 4, 2023

Choose a reason for hiding this comment

hawkingrei Apr 5, 2023

Choose a reason for hiding this comment

hawkingrei commented Apr 5, 2023

XuHuaiyu commented Apr 6, 2023

XuHuaiyu commented Apr 6, 2023

hawkingrei commented Apr 6, 2023

XuHuaiyu commented Apr 7, 2023

ti-chi-bot commented Apr 7, 2023

ti-chi-bot commented Apr 7, 2023

XuHuaiyu commented Apr 4, 2023 •

edited

Loading

ti-chi-bot commented Apr 4, 2023 •

edited

Loading