Skip to content
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.

Sign up for GitHub

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

Jump to bottom

*: sync wait stats loading for stable plan #30026

Merged
merged 64 commits into from
Dec 31, 2021
Merged

*: sync wait stats loading for stable plan #30026

merged 64 commits into from
Dec 31, 2021

Conversation

chrysan
Copy link
Contributor

@chrysan chrysan commented Nov 23, 2021 •
edited by Yisaer
Loading

What problem does this PR solve?

Issue Number: close #37434

Problem Summary:
Plans are unstable due to async lazy-load of statistics. Sometimes, unreasonable plans cause oom.

What is changed and how it works?

Make SQL goroutine sync-wait for the stats loading.

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

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

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Nov 23, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • winoros
  • xuyifangreeneyes

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.

@ti-chi-bot 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 23, 2021
@ti-chi-bot ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 25, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2021
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2021
tiancaiamao
tiancaiamao reviewed Dec 11, 2021
View reviewed changes
Copy link
Contributor

@tiancaiamao tiancaiamao left a comment

Choose a reason for hiding this comment

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

Please ignore my review, I just go over the Go language and timer part,
I never take a close look at the statistics code logic.

@chrysan
Copy link
Contributor Author

chrysan commented Dec 30, 2021

/run-unit-test

1 similar comment
@chrysan
Copy link
Contributor Author

chrysan commented Dec 30, 2021

/run-unit-test

@chrysan
Copy link
Contributor Author

chrysan commented Dec 30, 2021

/run-unit-test

@chrysan
Copy link
Contributor Author

chrysan commented Dec 30, 2021

/run-unit-test

xuyifangreeneyes
xuyifangreeneyes approved these changes Dec 30, 2021
View reviewed changes
Copy link
Contributor

@xuyifangreeneyes xuyifangreeneyes left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 30, 2021
@ti-chi-bot 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 Dec 31, 2021
@winoros
Copy link
Member

winoros commented Dec 31, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 8abdb06

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 31, 2021
@chrysan
Copy link
Contributor Author

chrysan commented Dec 31, 2021

/run-mysql-test

@ti-chi-bot ti-chi-bot merged commit 48fce5e into pingcap:master Dec 31, 2021
hawkingrei
hawkingrei reviewed Dec 31, 2021
View reviewed changes
planner/core/plan_stats_test.go
"github.com/pingcap/tidb/util/testleak"
)

var _ = Suite(&testPlanStatsSuite{})
Copy link
Member

Choose a reason for hiding this comment

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

it should use testify in the current test case. check is retired.

@chrysan chrysan mentioned this pull request Jan 5, 2022
Merged
15 tasks
@xuyifangreeneyes xuyifangreeneyes mentioned this pull request Jan 7, 2022
Open
@chrysan chrysan mentioned this pull request Jan 17, 2022
Closed
@morgo morgo mentioned this pull request Mar 22, 2022
Merged
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiancaiamao tiancaiamao tiancaiamao left review comments

@hawkingrei hawkingrei hawkingrei left review comments

@qw4990 qw4990 qw4990 approved these changes

@winoros winoros winoros approved these changes

@xuyifangreeneyes xuyifangreeneyes xuyifangreeneyes approved these changes

@time-and-fate time-and-fate Awaiting requested review from time-and-fate

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sync stats in optimizer in order to make plan stable
9 participants
@chrysan @ti-chi-bot @dbsid @sre-bot @winoros @tiancaiamao @hawkingrei @qw4990 @xuyifangreeneyes