*: add a memdb memory tracker #35225

ekexium · 2022-06-08T07:27:31Z

Signed-off-by: ekexium ekexium@fastmail.com

What problem does this PR solve?

Issue Number: ref #35203

Problem Summary:

What is changed and how it works?

It replaces the EntriesSize in TxnInfo with a memory tracker of MemDB.
We set a hook of the memory change of memdb, thus the manual update of the field is unnecessary.
[TODO] An appropriate limit and an action haven't been set for the tracker. Before we decide how to control it, use total-txn-size-limit for now.

Relates to tikv/client-go#520

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

MySQL root@172.16.5.181:CREDIT_CARD> show config where name like '%txn-total%'
+------+-------------------+----------------------------------+------------+
| Type | Instance          | Name                             | Value      |
+------+-------------------+----------------------------------+------------+
| tidb | 172.16.5.181:4000 | performance.txn-total-size-limit | 1099511620 |
+------+-------------------+----------------------------------+------------+
1 row in set
Time: 0.067s
MySQL root@172.16.5.181:CREDIT_CARD> delete from t_1;
You're about to run a destructive command.
Do you want to proceed? (y/n): y
Your call!
(1105, 'Out Of Memory Quota![conn_id=0]')

The statement aborts when the memdb memory footprint reaches the limit (1.02G).

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

ti-chi-bot · 2022-06-08T07:27:33Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

XuHuaiyu
wshwsh12

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.

ekexium · 2022-06-08T09:27:58Z

/run-build

wshwsh12 · 2022-06-24T06:47:01Z

tests/realtikvtest/txntest/txn_state_test.go

@@ -119,7 +119,7 @@ func TestBasicTxnState(t *testing.T) {
 	require.Nil(t, info)
 }

-func TestEntriesCountAndSize(t *testing.T) {
+func TestEntriesCount(t *testing.T) {


Why not check Size?

Now size is checked by the new test because it now reflects the actual memory footprint and thus is not a fixed number.

tests/realtikvtest/txntest/txn_state_test.go

Signed-off-by: ekexium <ekexium@fastmail.com>

sre-bot · 2022-07-05T22:02:21Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ hawkingrei
✅ ti-chi-bot
❌ ekexium

ekexium seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

wshwsh12

Rest LGTM

wshwsh12 · 2022-11-21T09:39:10Z

executor/infoschema_reader.go

@@ -2607,7 +2607,12 @@ func (e *tidbTrxTableRetriever) retrieve(ctx context.Context, sctx sessionctx.Co
 						row = append(row, types.NewDatum(nil))
 					}
 				} else {
-					row = append(row, e.txnInfo[i].ToDatum(c.Name.O))
+					if c.Name.O == txninfo.MemBufferBytesStr {


I think use switch-case-default is better..

XuHuaiyu · 2022-11-22T06:56:02Z

session/session.go

+func (s *session) SetMemoryFootprintChangeHook() {
+	hook := func(mem uint64) {
+		if s.sessionVars.MemDBFootprint == nil {
+			tracker := memory.NewTracker(memory.LabelForMemDB, -1)


Is the total-txn-size-limit actually deprecated after this commit?

It's still used in memdb. But I think we could consider deprecating the config.

Signed-off-by: ekexium <eke@fastmail.com>

ekexium · 2022-11-23T06:59:19Z

There's a test failed for data race. Let's wait for tikv/client-go#621

Signed-off-by: ekexium <eke@fastmail.com>

ekexium · 2022-11-24T04:31:06Z

PTAL @XuHuaiyu

XuHuaiyu · 2022-11-24T06:10:28Z

/merge

ti-chi-bot · 2022-11-24T06:10:31Z

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

Commit hash: f1b87fc

ti-chi-bot · 2022-11-24T08:50:10Z

@ekexium: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

ekexium · 2022-11-24T10:08:08Z

I changed the email address of my git config and caused the CLA check failure. I've made a new PR #39372

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 8, 2022

ekexium force-pushed the memdb-tracker branch 2 times, most recently from bd16bfd to 2bda643 Compare June 8, 2022 08:57

ekexium requested a review from XuHuaiyu June 8, 2022 08:59

ekexium marked this pull request as ready for review June 8, 2022 09:27

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 Jun 8, 2022

ekexium mentioned this pull request Jun 8, 2022

Track the memory footprint of MemDB #35203

Closed

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

XuHuaiyu requested a review from wshwsh12 June 23, 2022 03:12

wshwsh12 reviewed Jun 24, 2022

View reviewed changes

ekexium and others added 5 commits June 24, 2022 16:36

add a memdb memory tracker

e40b5c5

Signed-off-by: ekexium <ekexium@fastmail.com>

update deps

07c297a

fix test

0d8de72

Signed-off-by: ekexium <ekexium@fastmail.com>

remove the fake tracking of memdb in insert

078fa86

Signed-off-by: ekexium <ekexium@fastmail.com>

simplify the signature

7348b60

Signed-off-by: ekexium <ekexium@fastmail.com>

ekexium force-pushed the memdb-tracker branch from 15e96d9 to ce95ccb Compare June 24, 2022 08:38

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

ekexium force-pushed the memdb-tracker branch from ce95ccb to fdbcc73 Compare June 24, 2022 08:57

fix conflict

befc8ee

Signed-off-by: ekexium <ekexium@fastmail.com>

ekexium force-pushed the memdb-tracker branch from fdbcc73 to befc8ee Compare June 24, 2022 09:53

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

Merge branch 'master' of github.com:pingcap/tidb into memdb-tracker

b0773cd

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

ekexium force-pushed the memdb-tracker branch 2 times, most recently from 0b661bd to 30a86ff Compare November 16, 2022 09:38

wshwsh12 approved these changes Nov 21, 2022

View reviewed changes

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

Merge branch 'master' into memdb-tracker

bb846ba

XuHuaiyu reviewed Nov 22, 2022

View reviewed changes

ekexium added 2 commits November 22, 2022 16:55

refactor: use switch-default

b8b5b89

Signed-off-by: ekexium <eke@fastmail.com>

fix: only reset session tracker when detach SQLText tracker

b01184c

Signed-off-by: ekexium <eke@fastmail.com>

ekexium force-pushed the memdb-tracker branch from 0e38f62 to bde3c82 Compare November 23, 2022 02:40

test: fix TestTiDBTrx

cb76015

Signed-off-by: ekexium <eke@fastmail.com>

ekexium force-pushed the memdb-tracker branch from bde3c82 to cb76015 Compare November 23, 2022 03:01

Merge branch 'master' into memdb-tracker

da25d0c

ekexium force-pushed the memdb-tracker branch 2 times, most recently from c2e303b to 33122d4 Compare November 23, 2022 12:03

update client-go

f1b87fc

Signed-off-by: ekexium <eke@fastmail.com>

ekexium force-pushed the memdb-tracker branch from 33122d4 to f1b87fc Compare November 24, 2022 03:19

XuHuaiyu approved these changes Nov 24, 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 Nov 24, 2022

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

ti-chi-bot added 4 commits November 24, 2022 14:10

Merge branch 'master' into memdb-tracker

412f56d

Merge branch 'master' into memdb-tracker

bf33f2e

Merge branch 'master' into memdb-tracker

2c0a679

Merge branch 'master' into memdb-tracker

74b29db

ekexium mentioned this pull request Nov 24, 2022

*: add a memdb memory tracker #39372

Merged

12 tasks

ekexium closed this Nov 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add a memdb memory tracker #35225

*: add a memdb memory tracker #35225

ekexium commented Jun 8, 2022 •

edited

Loading

ti-chi-bot commented Jun 8, 2022 •

edited

Loading

ekexium commented Jun 8, 2022

wshwsh12 Jun 24, 2022

ekexium Jun 24, 2022

sre-bot commented Jul 5, 2022 •

edited

Loading

wshwsh12 left a comment

wshwsh12 Nov 21, 2022

XuHuaiyu Nov 22, 2022

ekexium Nov 22, 2022

ekexium commented Nov 23, 2022

ekexium commented Nov 24, 2022

XuHuaiyu commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

ekexium commented Nov 24, 2022

*: add a memdb memory tracker #35225

*: add a memdb memory tracker #35225

Conversation

ekexium commented Jun 8, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jun 8, 2022 • edited Loading

ekexium commented Jun 8, 2022

wshwsh12 Jun 24, 2022

Choose a reason for hiding this comment

ekexium Jun 24, 2022

Choose a reason for hiding this comment

sre-bot commented Jul 5, 2022 • edited Loading

wshwsh12 left a comment

Choose a reason for hiding this comment

wshwsh12 Nov 21, 2022

Choose a reason for hiding this comment

XuHuaiyu Nov 22, 2022

Choose a reason for hiding this comment

ekexium Nov 22, 2022

Choose a reason for hiding this comment

ekexium commented Nov 23, 2022

ekexium commented Nov 24, 2022

XuHuaiyu commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

ekexium commented Nov 24, 2022

ekexium commented Jun 8, 2022 •

edited

Loading

ti-chi-bot commented Jun 8, 2022 •

edited

Loading

sre-bot commented Jul 5, 2022 •

edited

Loading