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: fix auto analyze for global index #47754

Conversation

L-maple
Copy link
Contributor

@L-maple L-maple commented Oct 18, 2023

What problem does this PR solve?

Issue Number: close #47539

Problem Summary:

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

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

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2023
@ti-chi-bot
Copy link

ti-chi-bot bot commented Oct 18, 2023

Hi @L-maple. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 kubernetes/test-infra repository.

@tiprow
Copy link

tiprow bot commented Oct 18, 2023

Hi @L-maple. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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 kubernetes/test-infra repository.

@hawkingrei
Copy link
Member

/ok-to-test

@ti-chi-bot ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Oct 19, 2023
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #47754 (67b89bc) into master (18979c6) will increase coverage by 1.3715%.
Report is 1402 commits behind head on master.
The diff coverage is 29.2682%.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #47754        +/-   ##
================================================
+ Coverage   71.4161%   72.7877%   +1.3715%     
================================================
  Files          1402       1425        +23     
  Lines        406373     412792      +6419     
================================================
+ Hits         290216     300462     +10246     
+ Misses        96261      93406      -2855     
+ Partials      19896      18924       -972     
Flag Coverage Δ
integration 42.9595% <29.2682%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 53.9874% <ø> (ø)
parser ∅ <ø> (∅)
br 48.6730% <ø> (-4.2509%) ⬇️

@qw4990
Copy link
Contributor

qw4990 commented Oct 30, 2023

@hawkingrei PTAL

@hawkingrei hawkingrei self-requested a review October 31, 2023 02:59
@hawkingrei
Copy link
Member

/test all

@hawkingrei
Copy link
Member

hawkingrei commented Oct 31, 2023

@L-maple Please run make bazel_prepare to update bazel config.

@L-maple L-maple force-pushed the feature/fix_autoanalyze_for_global_index branch from 32fec15 to c30805a Compare October 31, 2023 11:13
@ti-chi-bot ti-chi-bot bot added the needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. label Oct 31, 2023
@hawkingrei
Copy link
Member

/retest

Copy link

ti-chi-bot bot commented Oct 31, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawkingrei

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Oct 31, 2023
Copy link

ti-chi-bot bot commented Oct 31, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-10-31 13:57:42.762286088 +0000 UTC m=+2961460.349396233: ☑️ agreed by hawkingrei.

@winoros
Copy link
Member

winoros commented Nov 2, 2023

/hold for a while for some discussion

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2023
@winoros
Copy link
Member

winoros commented Nov 2, 2023

@L-maple, I think the changes in this pull will make the global index's statistics collecting a separate process from the merging global stats?

@L-maple L-maple force-pushed the feature/fix_autoanalyze_for_global_index branch from c30805a to dd3b48d Compare November 6, 2023 03:39
@L-maple
Copy link
Contributor Author

L-maple commented Nov 6, 2023

@L-maple, I think the changes in this pull will make the global index's statistics collecting a separate process from the merging global stats?

@winoros Thanks, I revise the PR again for merging global stats compatibility.
in handleGlobalStats, if results.TableID.IsPartitionTable() && needGlobalStats { will check the global index is not needed to be merged.

@L-maple L-maple force-pushed the feature/fix_autoanalyze_for_global_index branch from dd3b48d to 67b89bc Compare November 6, 2023 03:48
@L-maple
Copy link
Contributor Author

L-maple commented Nov 10, 2023

@winoros Hi, do you have any other advice?This PR can be reiviewed again.

@winoros
Copy link
Member

winoros commented Nov 13, 2023

@winoros Hi, do you have any other advice?This PR can be reiviewed again.

It needs some time to discuss the behavior. How could I reach you via some IM?

@L-maple
Copy link
Contributor Author

L-maple commented Nov 13, 2023 via email

@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2023
Copy link

ti-chi-bot bot commented Nov 17, 2023

PR needs rebase.

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 kubernetes/test-infra repository.

Copy link

ti-chi-bot bot commented Apr 10, 2024

@L-maple: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
idc-jenkins-ci-tidb/build 67b89bc link true /test build
idc-jenkins-ci-tidb/check_dev 67b89bc link true /test check-dev
idc-jenkins-ci-tidb/mysql-test 67b89bc link true /test mysql-test
idc-jenkins-ci-tidb/check_dev_2 67b89bc link true /test check-dev2
idc-jenkins-ci-tidb/unit-test 67b89bc link true /test unit-test
pull-integration-ddl-test 67b89bc link true /test pull-integration-ddl-test
pull-mysql-client-test 67b89bc link true /test pull-mysql-client-test
pull-br-integration-test 67b89bc link true /test pull-br-integration-test
pull-lightning-integration-test 67b89bc link true /test pull-lightning-integration-test

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

@Defined2014 Defined2014 closed this Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-1-more-lgtm Indicates a PR needs 1 more LGTM. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

autoanalyze failed when a table has global index, analyze version = 1
5 participants