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.

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

statistics: enables global-level stats to be generated in fast analyze mode #22931

Merged
merged 20 commits into from
Mar 1, 2021

Conversation

Reminiscent
Copy link
Contributor

@Reminiscent Reminiscent commented Feb 25, 2021

What problem does this PR solve?

sub PR for #22554

Problem Summary:

  1. Generate the global-level stats when we analyze the partition table in fast analyze mode.
  2. Fix an issue for globalStats.Count.

What is changed and how it works?

Same as described above.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM
  • Breaking backward compatibility

Release note

  • No release note

@Reminiscent Reminiscent requested review from a team as code owners February 25, 2021 02:57
@Reminiscent Reminiscent requested review from lzmhhh123 and removed request for a team February 25, 2021 02:57
@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 25, 2021
@ti-chi-bot ti-chi-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 25, 2021
@Reminiscent Reminiscent reopened this Feb 25, 2021
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 25, 2021
@Reminiscent
Copy link
Contributor Author

/run-all-tests

@@ -171,6 +171,12 @@ func (e *AnalyzeExec) Next(ctx context.Context, req *chunk.Chunk) error {
sc := e.ctx.GetSessionVars().StmtCtx
globalStats, err := statsHandle.MergePartitionStats2GlobalStats(sc, infoschema.GetInfoSchema(e.ctx), globalStatsID.tableID, info.isIndex, info.idxID)
if err != nil {
errMessage := err.Error()
if errMessage == "[stats] build global-level stats failed due to missing partition-level stats" {
Copy link
Contributor

@qw4990 qw4990 Feb 26, 2021

Choose a reason for hiding this comment

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

Judging an error by comparing its error message is dangerous.
You'd better create a new kind of error and use errors.Cause(err).
Here is an example:

			if _, ok := errors.Cause(err).(*tikv.PDError); !ok {
				break
			}

@@ -809,8 +815,7 @@ func (e *AnalyzeFastExec) calculateEstimateSampleStep() (err error) {
sql := new(strings.Builder)
sqlexec.MustFormatSQL(sql, "select count(*) from %n.%n", dbInfo.Name.L, e.tblInfo.Name.L)

pruneMode := variable.PartitionPruneMode(e.ctx.GetSessionVars().PartitionPruneMode.Load())
if pruneMode != variable.DynamicOnly && e.tblInfo.ID != e.tableID.GetStatisticsID() {
if e.tblInfo.ID != e.tableID.GetStatisticsID() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we check PartitionPruneMode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AnalyzeFastExec structure is based on partition. The content recorded in it corresponds to the content of the partition, not the whole table.

@ti-chi-bot
Copy link
Member

@qw4990: Please use /LGTM instead of LGTM when you want to approve the pull request by comment.
If you use the GitHub review feature, please approve the PR directly, the comment will not take effect in the GitHub review feature.
If you have any qustions please refer to lgtm command help or lgtm plugin design.

If you have approved this PR, please ignore this reply. This reply is being used as a temporary reply during the migration of the new bot and will be removed on April 1.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

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

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

@Reminiscent
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@Reminiscent: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

@Reminiscent: /merge in this pull request requires 2 /lgtm.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@Reminiscent
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@Reminiscent: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

@Reminiscent: /merge in this pull request requires 2 /lgtm.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@Reminiscent
Copy link
Contributor Author

/run-tics-test

@Reminiscent
Copy link
Contributor Author

/run-integration-common-test

@Reminiscent
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@Reminiscent: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

@Reminiscent: /merge in this pull request requires 2 /lgtm.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@qw4990
Copy link
Contributor

qw4990 commented Mar 1, 2021

/merge

@ti-chi-bot
Copy link
Member

@qw4990: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

@qw4990: /merge in this pull request requires 2 /lgtm.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

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

@qw4990 qw4990 merged commit 550ca8e into pingcap:master Mar 1, 2021
@Reminiscent Reminiscent deleted the fastAnalyze branch August 5, 2021 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression component/statistics sig/execution SIG execution size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants