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

sessionctx: add variable tidb_analyze_version #21515

Merged
merged 3 commits into from
Dec 8, 2020
Merged

sessionctx: add variable tidb_analyze_version #21515

merged 3 commits into from
Dec 8, 2020

Conversation

winoros
Copy link
Member

@winoros winoros commented Dec 6, 2020

What problem does this PR solve?

Problem Summary:

Add a variable tidb_analyze_version to control how we collect and use the analyzed statistics.

Check List

Tests

  • Unit test

Release note

  • No release note

@morgo
Copy link
Contributor

morgo commented Dec 7, 2020

I think there could be some confusion with tidb_enable_fast_analyze. Is it possible that the new setting could merge both variables, or explain the difference better?

@winoros winoros added the sig/planner SIG: Planner label Dec 7, 2020
@time-and-fate time-and-fate mentioned this pull request Dec 7, 2020
Closed
@winoros
Copy link
Member Author

winoros commented Dec 8, 2020 •
edited
Loading

The following stuff will be added to the docs for clearer explanation:

The enable_fast_analyze means use sampling to collect index statistics.
The analyze_version means how we collect i.e. how we sampling or how we deal with the sampling or the structure of the statistics may be changed. This variable can control it for testing or rolling back.

qw4990
qw4990 reviewed Dec 8, 2020
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 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-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 8, 2020
lzmhhh123
lzmhhh123 reviewed Dec 8, 2020
View reviewed changes
Copy link
Contributor

@lzmhhh123 lzmhhh123 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-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 8, 2020
ti-srebot
ti-srebot previously approved these changes Dec 8, 2020
View reviewed changes
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 8, 2020
time-and-fate
time-and-fate reviewed Dec 8, 2020
View reviewed changes
sessionctx/variable/tidb_vars.go
@@ -601,6 +604,7 @@ const (
DefTiDBEnableAsyncCommit = false
DefTiDBEnable1PC = false
DefTiDBGuaranteeExternalConsistency = false
DefTiDBAnalyzeVersion = 1
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we'll use old stats by default and change it manually to enable refactored stats?

Copy link
Member Author

@winoros winoros Dec 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, according to the PM, it should be turned to the newer one after we have fully tested it.

@winoros
Copy link
Member Author

winoros commented Dec 8, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 8, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit b269ef8 into pingcap:master Dec 8, 2020
@winoros winoros deleted the var-analyze-version branch December 8, 2020 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@time-and-fate time-and-fate time-and-fate left review comments

@lzmhhh123 lzmhhh123 lzmhhh123 left review comments

@qw4990 qw4990 qw4990 approved these changes

@ti-srebot ti-srebot ti-srebot left review comments

Assignees
No one assigned
Labels
sig/planner SIG: Planner 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.
6 participants
@winoros @morgo @ti-srebot @qw4990 @time-and-fate @lzmhhh123