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

stats: fix panic when init stats for cm sketch #14421

Merged
merged 4 commits into from
Jan 14, 2020
Merged

stats: fix panic when init stats for cm sketch #14421

merged 4 commits into from
Jan 14, 2020

Conversation

alivxxx
Copy link
Contributor

@alivxxx alivxxx commented Jan 9, 2020

What problem does this PR solve?

Currently, when init stats, we use different txn to read stats for meta, histograms etc.
So it is possible that when init stats:

  • We first read the histogram and cm sketches, and the table is not analyzed, so there is nothing in cm sketch.
  • Analyze is triggered on the table.
  • Read the topn items, there are some values in it for the table.
  • Decode the cm sketch using the cm sketch using CMSketchFromProto, and it panics because there is no rows in cm sketch.

What is changed and how it works?

  • Move the check of number of rows for cm sketch into CMSketchFromProto to avoid panic.
  • Refactor the init stats and read table stats, so they always read stats using the same txn.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • None

Related changes

  • Need to cherry-pick to the release branch

Release note

  • fix panic when init stats for topn items.

@alivxxx alivxxx requested a review from a team as a code owner January 9, 2020 07:50
@ghost ghost removed their request for review January 9, 2020 08:19
eurekaka
eurekaka reviewed Jan 9, 2020
View reviewed changes
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

statistics/handle/handle.go
@@ -366,10 +378,8 @@ func (h *Handle) indexStatsFromStorage(row chunk.Row, table *statistics.Table, t
errorRate := statistics.ErrorRate{}
flag := row.GetInt64(8)
lastAnalyzePos := row.GetDatum(10, types.NewFieldType(mysql.TypeBlob))
if statistics.IsAnalyzed(flag) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the error rate should not be cleaned when using snapshot read.

@alivxxx alivxxx requested a review from winoros January 9, 2020 09:01
winoros
winoros approved these changes Jan 14, 2020
View reviewed changes
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@alivxxx alivxxx added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. needs-cherry-pick-3.0 labels Jan 14, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 14, 2020

/run-all-tests

@sre-bot sre-bot merged commit 253a81e into pingcap:master Jan 14, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 14, 2020

cherry pick to release-3.0 failed

@alivxxx alivxxx deleted the init branch January 14, 2020 06:25
alivxxx added a commit to alivxxx/tidb that referenced this pull request Jan 14, 2020
@alivxxx
tats: fix panic when init stats for cm sketch (pingcap#14421)
0cb5f9a
@alivxxx alivxxx mentioned this pull request Jan 14, 2020
Merged
@bb7133 bb7133 modified the milestone: v3.0.10 Jan 15, 2020
zz-jason pushed a commit that referenced this pull request Jan 19, 2020
@alivxxx @zz-jason
stats: fix panic when init stats for cm sketch (#14421) (#14470)
2ddeb25
@xuyifangreeneyes xuyifangreeneyes mentioned this pull request Feb 20, 2023
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eurekaka eurekaka eurekaka left review comments

@winoros winoros winoros approved these changes

@lzmhhh123 lzmhhh123 Awaiting requested review from lzmhhh123

Assignees
No one assigned
Labels
component/statistics status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alivxxx @sre-bot @eurekaka @winoros @bb7133