-
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
*: support auto analyze partition table #7789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
sql := fmt.Sprintf("alter table %s analyze partition `%s`", tblName, def.Name.O) | ||
statsTbl := h.GetPartitionStats(tblInfo, def.ID) | ||
analyzed, err := h.autoAnalyzeTable(tblInfo, statsTbl, start, end, autoAnalyzeRatio, sql) | ||
if analyzed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we analyze all partitions at once instead of only one partition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think only one partition is better because the partition stats is independent.
PTAL @zz-jason |
planner/core/planbuilder.go
Outdated
} | ||
} | ||
if !found { | ||
return nil, errors.New(fmt.Sprintf("Error in list of partitions to %s", tblInfo.Name.O)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about: "can not found the specified partition name xxx in the table definition"?
statsTbl := h.GetPartitionStats(tblInfo, def.ID) | ||
analyzed, err := h.autoAnalyzeTable(tblInfo, statsTbl, start, end, autoAnalyzeRatio, sql) | ||
if analyzed { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if one of the partition is analyzed, the rest of the partitions can never get a change to be analyzed, I think we should continue to analyze other partitions instead just return and terminate the analyze command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason that we only trigger one analyze once a time is that we can get the most updated auto analyze parameters. The rest of the partition can wait for the next round which is just 3s after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can get the most updated auto analyze parameters.
What does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In https://github.com/pingcap/tidb/blob/master/statistics/update.go#L654, we get the parameters like analyze time period, so if we continue analyze other tables, we may not use the latest parameters.
statistics/update.go
Outdated
continue | ||
} | ||
for _, def := range pi.Definitions { | ||
sql := fmt.Sprintf("alter table %s analyze partition `%s`", tblName, def.Name.O) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's wired that mysql uses the alter table
statement to analyze the table partitions... https://dev.mysql.com/doc/refman/5.7/en/partitioning-maintenance.htm
despite the compatible issue, can we also support another syntax to analyze table partitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can also support syntax like analyze table t partition p
.
Conflicts detected with this refactor commit: #7879 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
What problem does this PR solve?
Let auto analyze work for partition table.
What is changed and how it works?
Support analyze table for a specific partition, the syntax is
alter table t analyze partition a, b index c, d
oranalyze table t partition a, b index c, d
Check List
Tests
Code changes
Side effects
Related changes