-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
topsql: fix memory leak in stmtstats #34558
Conversation
Signed-off-by: mornyx <[email protected]>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
/cc @zhongzc @crazycs520 PTAL, thanks! |
How about tests to avoid regression? |
Signed-off-by: mornyx <[email protected]>
It was my fault in code review, I updated the unit tests but only covered this PR scenario. It is difficult to expose all possible memory leaks through unit or integration tests, which require long runs.. But I think mistakes like "missing some changes" can be avoided with more careful reviews, rather than being exposed until the QA stage. |
Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/3beef075a8e6a0cc35047171956c3fd61da3280f |
/assign @crazycs520 Please merge, thanks |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: be524ec
|
TiDB MergeCI notify
|
Signed-off-by: mornyx [email protected]
What problem does this PR solve?
Issue Number: ref #34525, #34502
In #33861, we also call the stmtstats framework to record the execution of statements when TopSQL is not enabled, but the aggregator inside stmtstats will take data only when TopSQL is enabled. So the memory leak happens.
In this PR, the behavior becomes: even if TopSQL is not enabled, the aggregator in stmtstats will take the data, but just discard it, not send it to the reporter.
Release note