analyze: fix AnalyzeV2 with global index #54420

Defined2014 · 2024-07-03T10:02:04Z

What problem does this PR solve?

Issue Number: close #54233

Problem Summary: Supports processing global index in analyze version V2. Because the data of the global index is not scattered in every partitions, only one analyze is required for the whole table.

Because we use an independent index analyze task, there will be results of the same version when saving, which needs to be processed like mv index.

After the analysis is complete, the global index will only have global stats.

What changed and how does it work?

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

Release note

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

None

ti-chi-bot · 2024-07-03T10:02:07Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tiprow · 2024-07-03T10:02:23Z

Hi @Defined2014. 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-sigs/prow repository.

codecov · 2024-07-03T10:19:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.2539%. Comparing base (95edc2d) to head (8bee4e5).
Report is 37 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #54420         +/-   ##
=================================================
- Coverage   72.8095%   56.2539%   -16.5556%     
=================================================
  Files          1533       1663        +130     
  Lines        435505     614257     +178752     
=================================================
+ Hits         317089     345544      +28455     
- Misses        98790     245244     +146454     
- Partials      19626      23469       +3843

Flag	Coverage Δ
integration	`37.0583% <94.4444%> (?)`
unit	`71.8246% <100.0000%> (+0.0106%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9656% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.5948% <ø> (+6.4751%)`	⬆️

ti-chi-bot · 2024-07-04T03:05:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign fixdb for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

hi-rustin

Thanks! 🤟 🖤

I have some questions(not only for you, I am still learning, so any comments from anyone all help):

What happens if the stats version is 1?
What happens if we only analyze a few partitions of one table in one analyze statement? Do we analyze the global index every time? What happens if the partition's stats are outdated, but the global index stats are updated?

I would like to ask @winoros and @time-and-fate those experts to help review this PR. 👀

hi-rustin · 2024-07-05T07:00:12Z

pkg/executor/analyze_idx.go

@@ -88,8 +88,8 @@ func analyzeIndexPushdown(idxExec *AnalyzeIndexExec) *statistics.AnalyzeResults
 		Count:    cnt,
 		Snapshot: idxExec.snapshot,
 	}
-	if idxExec.idxInfo.MVIndex {
-		result.ForMVIndex = true
+	if idxExec.idxInfo.MVIndex || (idxExec.idxInfo.Global && statsVer == statistics.Version2) {


May I ask what is the current behavior when the stats version is 1?

The ForMVIndex flag is used in update stats_meta table in SaveTableStatsToStorage function.

And for stats version V1, it should update stats_meta table directly no matter it already have a record or not.

And for stats version V2, it will check do we have a newer stats or not, if the result is not newer than orignal one, we will ignore it. Because in general, stats v2 will only have one analyze result. But this is problematic for mv index and global index. And mv index can't work with stats version V1, so only the global index needs to be checked.

hi-rustin · 2024-07-05T07:02:39Z

pkg/planner/core/planbuilder.go

@@ -2181,6 +2181,10 @@ func getModifiedIndexesInfoForAnalyze(
 		if originIdx.State != model.StatePublic {
 			continue
 		}
+		// For global index, we also need to analyze it as independent index analyze task.


Please update the comments above this function.

hi-rustin · 2024-07-05T07:09:35Z

pkg/planner/core/planbuilder.go

@@ -2327,6 +2331,13 @@ func (b *PlanBuilder) buildAnalyzeFullSamplingTask(
 		}
 	}

+	// Generate global indexes idxTask.
+	for _, indexInfo := range tbl.TableInfo.Indices {


How about we collect this information in the getModifiedIndexesInfoForAnalyze. Because in that function we:

collect the normal indexes

skip and collect the MVIndexes

skip the global index

How about we also collect the global indexes in this function? Then we don't need to iterate the indexes again. And also helps us do the index check in one place.

hi-rustin · 2024-07-05T07:10:41Z

pkg/statistics/analyze.go

@@ -108,7 +108,10 @@ type AnalyzeResults struct {
 	// take care of those table-level fields.
 	// In conclusion, when saving the analyze result for mv index, we need to store the index stats, as for the
 	// table-level fields, we only need to update the version.
-	ForMVIndex bool


I believe there are still some comments using ForMVIndex.

Defined2014 · 2024-07-05T08:30:00Z

Thanks! 🤟 🖤

I have some questions(not only for you, I am still learning, so any comments from anyone all help):

What happens if the stats version is 1?

What happens if we only analyze a few partitions of one table in one analyze statement? Do we analyze the global index every time? What happens if the partition's stats are outdated, but the global index stats are updated?

I would like to ask @winoros and @time-and-fate those experts to help review this PR. 👀

For stats version 1, it also calls generateIndexTasks funtion to generate global index tasks. Except for the key range, everything else is the same as the normal index, so there is not much to change.
We will analyze the global index every time.

winoros · 2024-07-09T13:48:01Z

pkg/planner/core/planbuilder.go

@@ -2169,18 +2169,24 @@ func getColOffsetForAnalyze(colsInfo []*model.ColumnInfo, colID int64) int {
 // in the execution phase of ANALYZE, we need to modify index.Columns[i].Offset according to colInfos.
 // TODO: find a better way to find indexed columns in ANALYZE rather than use IndexColumn.Offset
 // For multi-valued index, we need to collect it separately here and analyze it as independent index analyze task.
+// For global index, we also need to analyze it as independent index analyze task.


Add more comments for it. Unlike other special handled indexes. The scope of collecting is also changed for the global index.
Other special indexes are still collected in the scope of partition. However, the global index is collected in the scope of the whole table.

But I don't see any place to handle the real special part of the global index.

winoros · 2024-07-09T13:48:49Z

pkg/statistics/analyze.go

+	//
+	// The global index has only one key range, so an independent task is used to process it.
+	// Global index needs to update only the version at the table-level fields, just like mv index.
+	ForMVIndexOrGlobalIndex bool


It should be a enum or bitmask, not a bool.

winoros · 2024-07-09T13:52:36Z

For example, the global scope is conflicted with for each partition.

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels Jul 3, 2024

Defined2014 marked this pull request as ready for review July 3, 2024 10:02

ti-chi-bot bot added sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 3, 2024

Defined2014 added 2 commits July 3, 2024 18:46

analyze: fix analyze with global index

7cfa42e

update

f7a3bf0

Defined2014 force-pushed the fix-54233 branch from 68488ea to f7a3bf0 Compare July 3, 2024 10:47

hawkingrei self-requested a review July 3, 2024 11:38

update

4481095

Defined2014 changed the title ~~analyze: fix analyze with global index~~ analyze: fix AnalyzeV2 with global index Jul 4, 2024

update

3612279

update

4c484d1

Defined2014 requested review from qw4990, AilinKid, time-and-fate and hi-rustin July 4, 2024 05:28

hi-rustin reviewed Jul 5, 2024

View reviewed changes

update

8bee4e5

Defined2014 force-pushed the fix-54233 branch from ca3e3e9 to 8bee4e5 Compare July 5, 2024 07:52

winoros reviewed Jul 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

analyze: fix AnalyzeV2 with global index #54420

analyze: fix AnalyzeV2 with global index #54420

Defined2014 commented Jul 3, 2024 •

edited

Loading

ti-chi-bot bot commented Jul 3, 2024

tiprow bot commented Jul 3, 2024

codecov bot commented Jul 3, 2024 •

edited

Loading

ti-chi-bot bot commented Jul 4, 2024

hi-rustin left a comment •

edited

Loading

hi-rustin Jul 5, 2024

Defined2014 Jul 5, 2024 •

edited

Loading

hi-rustin Jul 5, 2024

hi-rustin Jul 5, 2024

hi-rustin Jul 5, 2024

Defined2014 commented Jul 5, 2024 •

edited

Loading

winoros Jul 9, 2024

winoros Jul 9, 2024

winoros Jul 9, 2024

winoros commented Jul 9, 2024

analyze: fix AnalyzeV2 with global index #54420

Are you sure you want to change the base?

analyze: fix AnalyzeV2 with global index #54420

Conversation

Defined2014 commented Jul 3, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Jul 3, 2024

tiprow bot commented Jul 3, 2024

codecov bot commented Jul 3, 2024 • edited Loading

Codecov Report

ti-chi-bot bot commented Jul 4, 2024

hi-rustin left a comment • edited Loading

Choose a reason for hiding this comment

hi-rustin Jul 5, 2024

Choose a reason for hiding this comment

Defined2014 Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

hi-rustin Jul 5, 2024

Choose a reason for hiding this comment

hi-rustin Jul 5, 2024

Choose a reason for hiding this comment

hi-rustin Jul 5, 2024

Choose a reason for hiding this comment

Defined2014 commented Jul 5, 2024 • edited Loading

winoros Jul 9, 2024

Choose a reason for hiding this comment

winoros Jul 9, 2024

Choose a reason for hiding this comment

winoros Jul 9, 2024

Choose a reason for hiding this comment

winoros commented Jul 9, 2024

Defined2014 commented Jul 3, 2024 •

edited

Loading

codecov bot commented Jul 3, 2024 •

edited

Loading

hi-rustin left a comment •

edited

Loading

Defined2014 Jul 5, 2024 •

edited

Loading

Defined2014 commented Jul 5, 2024 •

edited

Loading