*: support global memory control for tidb #37794

wshwsh12 · 2022-09-14T03:16:08Z

What problem does this PR solve?

Issue Number: ref #37816

Problem Summary:

What is changed and how it works?

Add system variable tidb_server_memory_limit and tidb_server_memory_limit_sess_min_size
When tidb memory usage is larger than limit, kill the memory usage top1 sql.
After the top1 sql is killed, try to GC immediately.

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.

Support global memory control

ti-chi-bot · 2022-09-14T03:16:09Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

XuHuaiyu
tiancaiamao

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.

XuHuaiyu · 2022-09-19T03:30:50Z

util/expensivequery/expensivequery.go

@@ -57,6 +58,7 @@ func (eqh *Handle) Run() {
 	defer ticker.Stop()
 	sm := eqh.sm.Load().(util.SessionManager)
 	record := &memoryUsageAlarm{}
+	serverMemoryQuota := &serverMemoryQuota{}


Do we need to use an individual goroutine to monitor the global memory status? The expensive_query worker handles too many things

util/servermemorylimit/servermemoryquota.go

util/memory/tracker.go

util/servermemorylimit/servermemoryquota.go

tiancaiamao · 2022-09-22T06:42:34Z

executor/executor_test.go

+	tracker3.Consume(300 << 20) // 300 MB
+
+	test := make([]int, 128<<20) // Keep 1GB HeapInUse
+	time.Sleep(500 * time.Millisecond)


What's the purpose of this Sleep?

The check goroutine checks the memory usage every 100ms. The Sleep() make sure that Top1Tracker can be Canceled.

It's better to add some comment for it.
Sleep slows down the test, and 0.5s is long enough for UT

tiancaiamao · 2022-09-22T06:45:12Z

executor/executor_test.go

+	tracker2.Consume(200 << 20) // 200 MB
+	tracker3.Consume(300 << 20) // 300 MB
+
+	test := make([]int, 128<<20) // Keep 1GB HeapInUse


This is virtual memory instead of real allocation before you visit the data.
OS will handle the logical -> physical memory address mapping and allocate the physical pages.

This only simulates the situation of heapinuse growth.
In normally, TiDB will use the memory immediately after allocation.

tiancaiamao · 2022-09-22T06:46:29Z

executor/executor_test.go

+			require.True(t, strings.Contains(r.(string), "Out Of Memory Quota!"))
+		})
+	tracker2.Consume(300 << 20) // Sum 500MB, Not Panic, Waiting t3 cancel finish.
+	time.Sleep(500 * time.Millisecond)


It takes time to cancel?

Wait for cancel

tiancaiamao · 2022-09-22T06:51:00Z

sessionctx/variable/sysvar.go

+		Validation: func(s *SessionVars, normalizedValue string, originalValue string, scope ScopeFlag) (string, error) {
+			intVal, err := strconv.ParseUint(normalizedValue, 10, 64)
+			if err != nil {
+				return "", err
+			}
+			if intVal > 0 && intVal < (512<<20) { // 512 MB
+				s.StmtCtx.AppendWarning(ErrTruncatedWrongValue.GenWithStackByArgs(TiDBServerMemoryLimit, originalValue))
+				intVal = 512 << 20
+			}
+			return strconv.FormatUint(intVal, 10), nil


Why not set MinValue at line 719?

0 means disable this feature.

OK... I see. This is kind of confusing
0 means disable, while when the value is smaller than 512MB, it's adjusted to 512MB (actual min value)

tiancaiamao · 2022-09-26T03:30:21Z

/merge

ti-chi-bot · 2022-09-26T03:30:25Z

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

Commit hash: 8fea589

sre-bot · 2022-09-26T04:37:48Z

TiDB MergeCI notify

🔴 Bad News! [1] CI still failing after this pr merged.
These failed integration tests don't seem to be introduced by the current PR.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-ddl-test	🔴 failed 2, success 4, total 6	29 min	Existing failure
idc-jenkins-ci/integration-cdc-test	🟢 all 37 tests passed	27 min	Existing passed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 17 tests passed	10 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	8 min 41 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	7 min 22 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	5 min 42 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 43 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 5 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 41 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

wshwsh12 added 6 commits September 2, 2022 17:51

temp

80f544f

support server-memory-limit-sess-min-size

92f8385

support config

a0e7727

Merge remote-tracking branch 'upstream/master' into server-memory-quota

9576c18

polish config && add some tests

bee18e2

support wait sql cancelled and GC

6f468c9

wshwsh12 added 2 commits September 14, 2022 15:29

Merge branch 'master' into server-memory-quota

87673bf

fix

a5a9053

wshwsh12 force-pushed the server-memory-quota branch from 869d21f to a5a9053 Compare September 14, 2022 07:51

wshwsh12 marked this pull request as ready for review September 14, 2022 08:54

wshwsh12 requested a review from a team as a code owner September 14, 2022 08:54

Merge branch 'master' into server-memory-quota

ea05d04

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2022

wshwsh12 force-pushed the server-memory-quota branch from 6df9ab3 to 5de3bac Compare September 14, 2022 09:18

fix

b198b25

wshwsh12 force-pushed the server-memory-quota branch from 5de3bac to b198b25 Compare September 14, 2022 09:36

wshwsh12 added 3 commits September 15, 2022 14:25

Merge remote-tracking branch 'upstream/master' into server-memory-quota

46a1b10

add ut

54ba480

fix

80c6f9e

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 Sep 15, 2022

Merge remote-tracking branch 'upstream/master' into server-memory-quota

3a19a02

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

XuHuaiyu reviewed Sep 19, 2022

View reviewed changes

wshwsh12 added 2 commits September 20, 2022 16:59

fix

e818081

Merge remote-tracking branch 'upstream/master' into server-memory-quota

a730889

wshwsh12 added 2 commits September 21, 2022 15:59

address comments

75d0191

address comments

9826ae0

wshwsh12 force-pushed the server-memory-quota branch from 8d80a03 to 9826ae0 Compare September 21, 2022 08:02

wshwsh12 added 2 commits September 21, 2022 16:06

remove useless check

667f838

fix bug

d2001fb

wshwsh12 requested a review from XuHuaiyu September 21, 2022 08:22

lint

3114a47

XuHuaiyu reviewed Sep 21, 2022

View reviewed changes

wshwsh12 added 3 commits September 22, 2022 00:15

address comments

1e46db7

address comments

cf5c4f7

Merge remote-tracking branch 'upstream/master' into server-memory-quota

089c6ac

XuHuaiyu approved these changes Sep 22, 2022

View reviewed changes

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

tiancaiamao reviewed Sep 22, 2022

View reviewed changes

wshwsh12 requested a review from tiancaiamao September 22, 2022 08:31

wshwsh12 mentioned this pull request Sep 23, 2022

Support global memory control #37816

Closed

9 tasks

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

Merge remote-tracking branch 'upstream/master' into server-memory-quota

29bf319

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

tiancaiamao approved these changes Sep 26, 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 Sep 26, 2022

add comment for time.sleep

8fea589

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

ti-chi-bot added 2 commits September 26, 2022 11:30

Merge branch 'master' into server-memory-quota

57c1ada

Merge branch 'master' into server-memory-quota

9da15d2

ti-chi-bot merged commit 4e4169b into pingcap:master Sep 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: support global memory control for tidb #37794

*: support global memory control for tidb #37794

wshwsh12 commented Sep 14, 2022 •

edited

Loading

ti-chi-bot commented Sep 14, 2022 •

edited

Loading

XuHuaiyu Sep 19, 2022

tiancaiamao Sep 22, 2022

wshwsh12 Sep 22, 2022

tiancaiamao Sep 26, 2022

tiancaiamao Sep 22, 2022

wshwsh12 Sep 22, 2022

tiancaiamao Sep 22, 2022

wshwsh12 Sep 22, 2022

tiancaiamao Sep 22, 2022

wshwsh12 Sep 22, 2022

tiancaiamao Sep 26, 2022

tiancaiamao commented Sep 26, 2022

ti-chi-bot commented Sep 26, 2022

sre-bot commented Sep 26, 2022

*: support global memory control for tidb #37794

*: support global memory control for tidb #37794

Conversation

wshwsh12 commented Sep 14, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 14, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented Sep 26, 2022

ti-chi-bot commented Sep 26, 2022

sre-bot commented Sep 26, 2022

TiDB MergeCI notify

wshwsh12 commented Sep 14, 2022 •

edited

Loading

ti-chi-bot commented Sep 14, 2022 •

edited

Loading