*: Allow Point_Get during DDL with Global Index #56382

mjonss · 2024-09-27T11:46:40Z

What problem does this PR solve?

Issue Number: ref #55819 , ref #45133

Problem Summary:
During DDL that affects Global Index, the PointGet access method is blocked, which may cause temporary bad query plans during partitioning management commands, like DROP/TRUNCATE/REORGANIZE PARTITION.

What changed and how does it work?

Allow PointGet, by filter away the partitions that should not be seen (like when the cleanup of the global index is ongoing).

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-09-27T11:46:47Z

[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 time-and-fate, wjhuang2016 for approval, ensuring that each of them provides their approval before proceeding. 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

tiprow · 2024-09-27T11:46:59Z

Hi @mjonss. 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-09-27T11:59:09Z

Codecov Report

Attention: Patch coverage is 83.48624% with 18 lines in your changes missing coverage. Please review.

Project coverage is 76.0517%. Comparing base (74034d4) to head (d4b94fc).

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #56382        +/-   ##
================================================
+ Coverage   73.3645%   76.0517%   +2.6871%     
================================================
  Files          1624       1646        +22     
  Lines        448069     456163      +8094     
================================================
+ Hits         328724     346920     +18196     
+ Misses        99207      87633     -11574     
- Partials      20138      21610      +1472

Flag	Coverage Δ
integration	`51.8282% <59.6330%> (?)`
unit	`72.4542% <75.2293%> (-0.0094%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`63.0723% <ø> (+17.5592%)`	⬆️

mjonss · 2024-09-27T15:15:20Z

pkg/meta/model/table.go

+// session should not see (may be duplicate errors on insert/update though)
+// For example during truncate or drop partition.
+func (pi *PartitionInfo) IDsInDDLToIgnore() []int64 {
+	// TODO:


There are multiple PRs/enhancements that will build upon this, especially for DROP and TRUNCATE partition.

mjonss · 2024-09-28T09:03:47Z

/retest

tiprow · 2024-09-28T09:04:09Z

@mjonss: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

mjonss · 2024-09-28T15:50:13Z

Splitted this part from #55819 since it can be reviewed separately.

…-get-during-ddl

Defined2014 · 2024-09-29T05:56:12Z

pkg/ddl/partition.go

-		ver, err = updateVersionAndTableInfo(jobCtx, t, job, tblInfo, originalState != job.SchemaState)
+		tblInfo.Partition.DDLState = job.SchemaState
+		tblInfo.Partition.DDLAction = job.Type
+		ver, err = updateVersionAndTableInfo(jobCtx, t, job, tblInfo, true)


I think it's better to keep originalState != job.SchemaState. Maybe we could ask DDL member to confirm it.

It will always be true, since originalState is set to job.SchemaState before the switch, and is changed in each of the switch cases before the call. And it should update the Schema Version for each of the state changes.

Defined2014 · 2024-09-29T05:58:14Z

pkg/planner/core/find_best_task.go

-			if canConvertPointGet {
-				if path != nil && path.Index != nil && path.Index.Global {
-					// Don't convert to point get during ddl
-					// TODO: Revisit truncate partition and global index


Can you explain why point get didn't work before?

Since it may have used partitions that should not have been seen, when it comes to global index, since the global index is unique it can be used for Point Get, and during DDL there might be old entries in the Global Index pointing to dropped partitions or new entries pointing to partitions being added or replacing old ones that are not yet seen by all sessions. Meaning a full table scan may see different data from a Global Index read.

Defined2014 · 2024-09-29T06:03:15Z

pkg/planner/core/find_best_task.go

 		args = append(args, expression.NewInt64Const(-1))
 	} else {
-		// `PartitionPruning`` func does not return adding and dropping partitions
+		// TODO: When PartitionPruning is guaranteed to not
+		// return old/blocked partition ids then ignoreMap can be removed.


Does it will happen? If the answer is yes, please add a related test case for it.

I'll add a test, I think there may be possible cases, until DROP/TRUNCATE/ADD PARTITION are fully updated for Global Index. Like #55831 and #56082.

I think idxArr will not contained any paritition in AddingDefinitions and DroppingDefinitions.

You are probably right, but I prefer to have all partitioning DDLs checked and tested for Global Index consistency and proper visibility for each state combination. So until then I prefer to have this check here. Or do you prefer to add an intest-assert in PartitionPruning instead?

Add an intest-assert is better for me.

Defined2014 · 2024-09-29T06:10:36Z

pkg/planner/core/find_best_task.go

 		}
 	}
+	if len(args) == 1 {


I think it only happens in the first if-branch, maybe we could move into it.

If one truncates all partitions or partition pruning don't find any matching partition, it could also happen in last if-branch.

…-get-during-ddl

qw4990 · 2024-09-30T03:09:09Z

pkg/executor/point_get.go

 				tblID = pid
-				if !matchPartitionNames(tblID, e.partitionNames, e.tblInfo.GetPartitionInfo()) {
+				pi := e.tblInfo.GetPartitionInfo()
+				if !matchPartitionNames(tblID, e.partitionNames, pi) {
 					return nil
 				}
+				for _, id := range pi.IDsInDDLToIgnore() {
+					if id == pid {
+						return nil
+					}
+				}


It seems better to converge all partition logic together into the PartitionProcessor.

Part of this is done in #56082, I created #56449 to make sure it is all handled.

…-get-during-ddl

*: Allow Point_Get during DDL with Global Index

82ed64a

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 27, 2024

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/planner SIG: Planner labels Sep 27, 2024

mjonss added 5 commits September 27, 2024 14:37

Added comment

ace4c13

Added back check for empty set of partitions after pruning

25d56a7

reshuffled code

e9424d8

Added todo comment

c4f0a13

Clarified comment

3a1271a

mjonss commented Sep 27, 2024

View reviewed changes

mjonss requested review from Defined2014 and qw4990 September 28, 2024 15:50

Merge remote-tracking branch 'pingcap/master' into global-index-point…

20aabb5

…-get-during-ddl

Defined2014 reviewed Sep 29, 2024

View reviewed changes

Merge remote-tracking branch 'pingcap/master' into global-index-point…

b7735b4

…-get-during-ddl

qw4990 reviewed Sep 30, 2024

View reviewed changes

mjonss added 3 commits October 4, 2024 13:10

Added intest assertion

5d0903e

Merge remote-tracking branch 'pingcap/master' into global-index-point…

3974c7e

…-get-during-ddl

Linting

d4b94fc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Allow Point_Get during DDL with Global Index #56382

*: Allow Point_Get during DDL with Global Index #56382

mjonss commented Sep 27, 2024

ti-chi-bot bot commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

codecov bot commented Sep 27, 2024 •

edited

Loading

mjonss Sep 27, 2024

mjonss commented Sep 28, 2024

tiprow bot commented Sep 28, 2024

mjonss commented Sep 28, 2024

Defined2014 Sep 29, 2024

mjonss Sep 29, 2024

Defined2014 Sep 29, 2024

mjonss Sep 29, 2024

Defined2014 Sep 29, 2024

mjonss Sep 29, 2024

Defined2014 Sep 29, 2024

mjonss Sep 29, 2024

Defined2014 Sep 30, 2024

Defined2014 Sep 29, 2024

mjonss Sep 29, 2024

qw4990 Sep 30, 2024

mjonss Oct 4, 2024

*: Allow Point_Get during DDL with Global Index #56382

Are you sure you want to change the base?

*: Allow Point_Get during DDL with Global Index #56382

Conversation

mjonss commented Sep 27, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

mjonss commented Sep 28, 2024

tiprow bot commented Sep 28, 2024

mjonss commented Sep 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2024 •

edited

Loading