Skip to content

statistics: add Destroy method and handle session recycling#59546

Merged
ti-chi-bot[bot] merged 23 commits intopingcap:masterfrom
0xPoe:rustin-patch-destroy
Feb 18, 2025
Merged

statistics: add Destroy method and handle session recycling#59546
ti-chi-bot[bot] merged 23 commits intopingcap:masterfrom
0xPoe:rustin-patch-destroy

Conversation

@0xPoe
Copy link
Copy Markdown
Member

@0xPoe 0xPoe commented Feb 14, 2025

What problem does this PR solve?

Issue Number: close #59524 close #59560

Problem Summary:

What changed and how does it work?

ref #59522

This is an alternative API design to solve the problem. I believe explicitly using destroy instead of exposing implementation details is a better approach to fix it.

I also added more logs to help us inspect error cases.

Note: This PR doesn’t address the root cause of the issues; it only ensures that we don’t encounter OOM problems. We still need to figure out why we’re running into this kind of stats loading.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix the issue that the internal session used by statistics may not be released and cause possible memory leak.
修复统计信息在使用的内部会话在遇到错误时可能没有被释放的问题,该问题可能导致内存泄漏。

Signed-off-by: Rustin170506 <techregister@pm.me>
…ntext

Signed-off-by: Rustin170506 <techregister@pm.me>
@ti-chi-bot ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 14, 2025
…ce and implement it in relevant structs

Signed-off-by: Rustin170506 <techregister@pm.me>
@ti-chi-bot ti-chi-bot 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 Feb 14, 2025
…ntext packages

Signed-off-by: Rustin170506 <techregister@pm.me>
Signed-off-by: Rustin170506 <techregister@pm.me>
…nd statistics packages

Signed-off-by: Rustin170506 <techregister@pm.me>
Signed-off-by: Rustin170506 <techregister@pm.me>
… decrement

Signed-off-by: Rustin170506 <techregister@pm.me>
Signed-off-by: Rustin170506 <techregister@pm.me>
@0xPoe
Copy link
Copy Markdown
Member Author

0xPoe commented Feb 14, 2025

/retest

…t cases

Signed-off-by: Rustin170506 <techregister@pm.me>
Signed-off-by: Rustin170506 <techregister@pm.me>
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 52.77778% with 34 lines in your changes missing coverage. Please review.

Project coverage is 74.9155%. Comparing base (f684d41) to head (8acba47).
Report is 36 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #59546        +/-   ##
================================================
+ Coverage   73.0249%   74.9155%   +1.8905%     
================================================
  Files          1693       1742        +49     
  Lines        468066     480776     +12710     
================================================
+ Hits         341805     360176     +18371     
+ Misses       105227      98081      -7146     
- Partials      21034      22519      +1485     
Flag Coverage Δ
integration 49.0800% <36.1111%> (?)
unit 72.3380% <52.7777%> (+0.1062%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 52.6910% <ø> (ø)
parser ∅ <ø> (∅)
br 61.0595% <ø> (+15.6137%) ⬆️

@0xPoe 0xPoe requested review from lance6716 and winoros February 14, 2025 08:50
Signed-off-by: Rustin170506 <techregister@pm.me>
@0xPoe
Copy link
Copy Markdown
Member Author

0xPoe commented Feb 14, 2025

Tested locally:

[2025/02/14 18:03:16.597 +08:00] [WARN] [domain.go:2619] ["load histograms failed"] [error="not implemented"] [errorVerbose="not implemented\ngithub.com/pingcap/tidb/pkg/statistics/handle/storage.(*statsReadWriter).LoadNeededHistograms.func1\n\t/Users/rustin/code/tidb/pkg/statistics/handle/storage/stats_read_writer.go:225\ngithub.com/pingcap/tidb/pkg/statistics/handle/util.WrapTxn\n\t/Users/rustin/code/tidb/pkg/statistics/handle/util/util.go:207\ngithub.com/pingcap/tidb/pkg/statistics/handle/util.CallWithSCtx\n\t/Users/rustin/code/tidb/pkg/statistics/handle/util/util.go:105\ngithub.com/pingcap/tidb/pkg/statistics/handle/storage.(*statsReadWriter).LoadNeededHistograms\n\t/Users/rustin/code/tidb/pkg/statistics/handle/storage/stats_read_writer.go:223\ngithub.com/pingcap/tidb/pkg/domain.(*Domain).asyncLoadHistogram\n\t/Users/rustin/code/tidb/pkg/domain/domain.go:2617\ngithub.com/pingcap/tidb/pkg/util.(*WaitGroupEnhancedWrapper).Run.func1\n\t/Users/rustin/code/tidb/pkg/util/wait_group_wrapper.go:103\nruntime.goexit\n\t/opt/homebrew/Cellar/go/1.23.6/libexec/src/runtime/asm_arm64.s:1223"]

Signed-off-by: Rustin170506 <techregister@pm.me>
Copy link
Copy Markdown
Member Author

@0xPoe 0xPoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔢 Self-check (PR reviewed by myself and ready for feedback.)

Comment thread pkg/statistics/handle/util/util.go
…grams

Signed-off-by: Rustin170506 <techregister@pm.me>
@0xPoe 0xPoe force-pushed the rustin-patch-destroy branch from 32ae7af to ca7c82f Compare February 14, 2025 14:46
@0xPoe 0xPoe added needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. labels Feb 18, 2025
@0xPoe
Copy link
Copy Markdown
Member Author

0xPoe commented Feb 18, 2025

/retest

1 similar comment
@0xPoe
Copy link
Copy Markdown
Member Author

0xPoe commented Feb 18, 2025

/retest

@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Feb 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qw4990, tangenta, tiancaiamao, time-and-fate, winoros

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@winoros
Copy link
Copy Markdown
Member

winoros commented Feb 18, 2025

/retest

@ti-chi-bot ti-chi-bot Bot merged commit 9f5f53a into pingcap:master Feb 18, 2025
ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 18, 2025
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Copy Markdown
Member

In response to a cherrypick label: new pull request created to branch release-8.5: #59614.
But this PR has conflicts, please resolve them!

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 18, 2025
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Copy Markdown
Member

In response to a cherrypick label: new pull request created to branch release-7.5: #59615.
But this PR has conflicts, please resolve them!

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 18, 2025
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Copy Markdown
Member

In response to a cherrypick label: new pull request created to branch release-8.1: #59616.
But this PR has conflicts, please resolve them!

@ti-chi-bot
Copy link
Copy Markdown
Member

In response to a cherrypick label: new pull request created to branch release-6.5: #59671.
But this PR has conflicts, please resolve them!

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 20, 2025
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot Bot removed the needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. label Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory leak risk in session pool usage in stats sync load release the internal session which may meet error like #54022

8 participants