executor: enhance global index #41197

L-maple · 2023-02-08T11:33:48Z

What problem does this PR solve?

Issue Number: close #40497
Issue Number: close #21732
Issue Number: close #40149

Note: PR #22478 doest not solve #40149 actually, so I list it here.

Problem Summary:

What is changed and how it works?

filter out the selected partitions in IndexLookUpExecutor when global index query specifies partitions.
only dynamic mode and global stats is ready, we can use the global index. Otherwise ignore it, as PartitionUnionAll logical plan not suitable for global index.
when drop global index's regions(drop table/index), we should specify the correct key prefix, like t{tableID}_i{indexID}.

Check List

Tests

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

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 · 2023-02-08T11:33:50Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Defined2014
mjonss

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2023-02-08T11:33:58Z

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.

L-maple · 2023-02-08T11:36:35Z

/label severity-moderate

ti-chi-bot · 2023-02-08T11:36:37Z

@L-maple: The label(s) severity-moderate cannot be applied. These labels are supported: fuzz/sqlancer, challenge-program, compatibility-breaker, first-time-contributor, contribution, require-LGT3, good first issue, correctness, duplicate, proposal, security, needs-more-info, needs-cherry-pick-release-4.0, needs-cherry-pick-release-5.0, needs-cherry-pick-release-5.1, needs-cherry-pick-release-5.2, needs-cherry-pick-release-5.3, needs-cherry-pick-release-5.4, needs-cherry-pick-release-6.0, needs-cherry-pick-release-6.1, needs-cherry-pick-release-6.2, needs-cherry-pick-release-6.3, needs-cherry-pick-release-6.4, needs-cherry-pick-release-6.5, affects-4.0, affects-5.0, affects-5.1, affects-5.2, affects-5.3, affects-5.4, affects-6.0, affects-6.1, affects-6.2, affects-6.3, affects-6.4, affects-6.5, may-affects-4.0, may-affects-5.0, may-affects-5.1, may-affects-5.2, may-affects-5.3, may-affects-5.4, may-affects-6.0, may-affects-6.1, may-affects-6.2, may-affects-6.3, may-affects-6.4, may-affects-6.5.

In response to this:

/label severity-moderate

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.

L-maple · 2023-02-08T11:37:27Z

/label severity/moderate

ti-chi-bot · 2023-02-08T11:37:29Z

@L-maple: The label(s) severity/moderate cannot be applied. These labels are supported: fuzz/sqlancer, challenge-program, compatibility-breaker, first-time-contributor, contribution, require-LGT3, good first issue, correctness, duplicate, proposal, security, needs-more-info, needs-cherry-pick-release-4.0, needs-cherry-pick-release-5.0, needs-cherry-pick-release-5.1, needs-cherry-pick-release-5.2, needs-cherry-pick-release-5.3, needs-cherry-pick-release-5.4, needs-cherry-pick-release-6.0, needs-cherry-pick-release-6.1, needs-cherry-pick-release-6.2, needs-cherry-pick-release-6.3, needs-cherry-pick-release-6.4, needs-cherry-pick-release-6.5, affects-4.0, affects-5.0, affects-5.1, affects-5.2, affects-5.3, affects-5.4, affects-6.0, affects-6.1, affects-6.2, affects-6.3, affects-6.4, affects-6.5, may-affects-4.0, may-affects-5.0, may-affects-5.1, may-affects-5.2, may-affects-5.3, may-affects-5.4, may-affects-6.0, may-affects-6.1, may-affects-6.2, may-affects-6.3, may-affects-6.4, may-affects-6.5.

In response to this:

/label severity/moderate

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.

L-maple · 2023-02-08T11:39:17Z

/cc @tiancaiamao @mjonss

L-maple · 2023-02-08T11:39:30Z

/assign @tiancaiamao

L-maple · 2023-02-08T11:46:52Z

/cc @xuyifangreeneyes

L-maple · 2023-02-08T12:01:55Z

/auto-cc

L-maple · 2023-02-10T06:36:07Z

/cc @bb7133

mjonss · 2023-02-15T10:18:12Z

/retest

ddl/index.go

L-maple · 2023-02-16T11:38:17Z

@L-maple please check if TestIngestMVIndexOnPartitionTable and TestIssue34228 is affected by this PR?

Maybe TestIngestMVIndexOnPartitionTable has no relation with my PR, which is always wrong.

Defined2014 · 2023-02-16T12:45:52Z

/retest-required

planner/core/logical_plan_builder.go

mjonss

LGTM, just some minor questions. Great work!

mjonss · 2023-02-17T17:10:17Z

ddl/sanity_check.go

@@ -97,7 +97,7 @@ func expectedDeleteRangeCnt(ctx delRangeCntCtx, job *model.Job) (int, error) {
 		if err := job.DecodeArgs(&startKey, &physicalTableIDs, &ruleIDs); err != nil {
 			return 0, errors.Trace(err)
 		}
-		return mathutil.Max(len(physicalTableIDs), 1), nil
+		return mathutil.Max(len(physicalTableIDs)+1, 1), nil


Why Max(x+1, 1) if x >= 0?

For drop table ddl the [t{tableID}_, t{tableID+1}) range is deleted in finishDDLJob, as logical table may contain global index.

I don't think the max function is needed anymore here. Since len(xx) is always >= 0, the return value of the max function is always equal to len(physicalTableIDs)+1. @L-maple

Suggested change

return mathutil.Max(len(physicalTableIDs)+1, 1), nil

return len(physicalTableIDs)+1, nil

@Defined2014 resolved, thanks.

executor/partition_table_test.go

mjonss · 2023-02-17T17:17:34Z

@L-maple Please ping me when you want to merge this into pingcap/tidb/master.

hawkingrei · 2023-02-19T06:58:19Z

/retest

hawkingrei · 2023-02-19T06:58:49Z

/retest-required

L-maple · 2023-02-20T09:20:34Z

@L-maple Please ping me when you want to merge this into pingcap/tidb/master.

@mjonss This PR can be merged, thanks for your advice above. There are still some bugs and nice features for global index. I will contribute more code for global index.

Defined2014 · 2023-02-21T03:21:37Z

@L-maple Please ping me when you want to merge this into pingcap/tidb/master.

@mjonss This PR can be merged, thanks for your advice above. There are still some bugs and nice features for global index. I will contribute more code for global index.

Let's wait till friday, if there is no news from mjonss, I will merge this PR. cc @mjonss

mjonss · 2023-02-21T05:04:59Z

/merge

ti-chi-bot · 2023-02-21T05:05:03Z

This pull request has been accepted and is ready to merge.

Commit hash: 7255cc1

L-maple · 2023-02-23T11:10:38Z

@mjonss Hi, mjonss! I just found the global index proposal was proposed in 2020.

Why it is delayed so long?
Is there any future plans for how to improve it in TiDB teams, and how long the global index will be a GA feature?
Could you give me some advice or prepared plans for improving the global index?

ti-chi-bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue labels Feb 8, 2023

L-maple force-pushed the feature/enhance_global_index_master branch from ed7b0ce to 85938c4 Compare February 8, 2023 11:37

ti-chi-bot requested review from mjonss and tiancaiamao February 8, 2023 11:39

ti-chi-bot assigned tiancaiamao Feb 8, 2023

ti-chi-bot requested a review from xuyifangreeneyes February 8, 2023 11:46

L-maple changed the title ~~executor: enhance global index~~ fix(executor): enhance global index Feb 8, 2023

L-maple changed the title ~~fix(executor): enhance global index~~ executor: enhance global index Feb 8, 2023

L-maple force-pushed the feature/enhance_global_index_master branch from 85938c4 to 05b6563 Compare February 9, 2023 03:09

L-maple mentioned this pull request Feb 10, 2023

executor: enhance global index #41159

Closed

12 tasks

ti-chi-bot requested a review from bb7133 February 10, 2023 06:36

mjonss reviewed Feb 15, 2023

View reviewed changes

ddl/index.go Outdated Show resolved Hide resolved

L-maple force-pushed the feature/enhance_global_index_master branch from 05b6563 to 3a5dd69 Compare February 15, 2023 11:13

L-maple force-pushed the feature/enhance_global_index_master branch 3 times, most recently from 9772e56 to 6b383c9 Compare February 16, 2023 12:08

L-maple force-pushed the feature/enhance_global_index_master branch from 6b383c9 to c808d37 Compare February 17, 2023 03:13

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 17, 2023

Defined2014 reviewed Feb 17, 2023

View reviewed changes

planner/core/logical_plan_builder.go Show resolved Hide resolved

Defined2014 approved these changes Feb 17, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 17, 2023

L-maple requested a review from mjonss February 17, 2023 08:45

mjonss approved these changes Feb 17, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 17, 2023

L-maple requested a review from mjonss February 20, 2023 09:17

*: enhance global index

7255cc1

L-maple force-pushed the feature/enhance_global_index_master branch from c808d37 to 7255cc1 Compare February 21, 2023 03:14

mjonss approved these changes Feb 21, 2023

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 21, 2023

ti-chi-bot merged commit 5cbf6eb into pingcap:master Feb 21, 2023

Defined2014 mentioned this pull request May 6, 2023

global index: drop global index or drop table but not delete global index range #40953

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: enhance global index #41197

executor: enhance global index #41197

L-maple commented Feb 8, 2023 •

edited

Loading

ti-chi-bot commented Feb 8, 2023 •

edited

Loading

ti-chi-bot commented Feb 8, 2023

L-maple commented Feb 8, 2023

ti-chi-bot commented Feb 8, 2023

L-maple commented Feb 8, 2023

ti-chi-bot commented Feb 8, 2023

L-maple commented Feb 8, 2023

L-maple commented Feb 8, 2023

L-maple commented Feb 8, 2023

L-maple commented Feb 8, 2023

L-maple commented Feb 10, 2023

mjonss commented Feb 15, 2023

L-maple commented Feb 16, 2023

Defined2014 commented Feb 16, 2023

mjonss left a comment

mjonss Feb 17, 2023

L-maple Feb 20, 2023

Defined2014 Feb 21, 2023 •

edited

Loading

L-maple Feb 21, 2023

mjonss commented Feb 17, 2023

hawkingrei commented Feb 19, 2023

hawkingrei commented Feb 19, 2023

L-maple commented Feb 20, 2023

Defined2014 commented Feb 21, 2023

mjonss commented Feb 21, 2023

ti-chi-bot commented Feb 21, 2023

L-maple commented Feb 23, 2023

	return mathutil.Max(len(physicalTableIDs)+1, 1), nil
	return len(physicalTableIDs)+1, nil

executor: enhance global index #41197

executor: enhance global index #41197

Conversation

L-maple commented Feb 8, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Feb 8, 2023 • edited Loading

ti-chi-bot commented Feb 8, 2023

L-maple commented Feb 8, 2023

ti-chi-bot commented Feb 8, 2023

L-maple commented Feb 8, 2023

ti-chi-bot commented Feb 8, 2023

L-maple commented Feb 8, 2023

L-maple commented Feb 8, 2023

L-maple commented Feb 8, 2023

L-maple commented Feb 8, 2023

L-maple commented Feb 10, 2023

mjonss commented Feb 15, 2023

L-maple commented Feb 16, 2023

Defined2014 commented Feb 16, 2023

mjonss left a comment

Choose a reason for hiding this comment

mjonss Feb 17, 2023

Choose a reason for hiding this comment

L-maple Feb 20, 2023

Choose a reason for hiding this comment

Defined2014 Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

L-maple Feb 21, 2023

Choose a reason for hiding this comment

mjonss commented Feb 17, 2023

hawkingrei commented Feb 19, 2023

hawkingrei commented Feb 19, 2023

L-maple commented Feb 20, 2023

Defined2014 commented Feb 21, 2023

mjonss commented Feb 21, 2023

ti-chi-bot commented Feb 21, 2023

L-maple commented Feb 23, 2023

L-maple commented Feb 8, 2023 •

edited

Loading

ti-chi-bot commented Feb 8, 2023 •

edited

Loading

Defined2014 Feb 21, 2023 •

edited

Loading