*: Truncate partition with global index improvement #55831

mjonss · 2024-09-03T14:40:33Z

What problem does this PR solve?

Issue Number: close #55819

Problem Summary:
During TRUNCATE PARTITION writes was blocked to partitions that was being truncated, also during StateDeleteReorganization, meaning the DDL would block writes to all truncated partitions for the full duration of the DDL, in case of a Global Index.

What changed and how does it work?

Instead of simply blocking writes to partitions being truncated, we are hiding the new partitions from Global Indexes StateWriteOnly and old partitions from StateDeleteOnly so that they cannot be seen, and not cause any anomaly, where you can find data or not depending on access method, i.e. it may be found if using Global Index, but not for table scan.

A side effect of this is that we are still blocking duplicate keys, if a write to a global unique index from a StateDeleteOnly client that collides with a not-yet cleaned up Global Index entry, or a write to a global unique index from StateWriteOnly (old partition, to be truncated) collides with a newly written entry in StateDeleteOnly (using the new emptied partition).

To not generate these kind of duplicate errors for too long period, duplicate key errors are not generated during StateDeleteReorganization if the duplicate comes from an old truncated partition, instead that global unique index entry is overwritten.

Also refactored/deduplicated code for cleaning up the global index entries from dropped/truncated partitions, so both DROP and TRUNCATE PARTITION uses the same code.

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.

Removed write blocking partitions involved in TRUNCATE PARTITIONs if they had a Global Index.

…tion-tests

…lobal-index

tiprow · 2024-09-03T14:40:57Z

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-03T15:33:09Z

Codecov Report

Attention: Patch coverage is 72.22222% with 65 lines in your changes missing coverage. Please review.

Project coverage is 57.0900%. Comparing base (5da9d1a) to head (ace2802).
Report is 1 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #55831         +/-   ##
=================================================
- Coverage   73.2077%   57.0900%   -16.1177%     
=================================================
  Files          1652       1803        +151     
  Lines        456113     642928     +186815     
=================================================
+ Hits         333910     367048      +33138     
- Misses       101690     250698     +149008     
- Partials      20513      25182       +4669

Flag	Coverage Δ
integration	`39.3748% <47.8632%> (?)`
unit	`72.5438% <72.2222%> (+0.0567%)`	⬆️

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

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

…-global-index

…uncate partition

mjonss · 2024-10-28T10:50:09Z

/retest

tiprow · 2024-10-28T10:50:30Z

@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.

…-global-index

mjonss · 2024-10-30T19:12:54Z

/retest

tiprow · 2024-10-30T19:13:15Z

@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.

…-global-index

purelind · 2024-11-01T15:34:19Z

/retest

tiprow · 2024-11-01T15:34:41Z

@purelind: 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.

winoros

for planner part

ti-chi-bot · 2024-11-01T16:13:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: winoros
Once this PR has been reviewed and has the lgtm label, please assign benjamin2037 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

ti-chi-bot · 2024-11-01T16:13:06Z

[LGTM Timeline notifier]

Timeline:

2024-11-01 16:13:06.31262172 +0000 UTC m=+625499.151777263: ☑️ agreed by winoros.

Defined2014 · 2024-11-02T07:34:24Z

pkg/table/tables/index.go

@@ -551,6 +583,32 @@ func FetchDuplicatedHandle(ctx context.Context, key kv.Key, distinct bool,
 	return true, nil, nil
 }

+// GetPartitionHandleFromVal creates a PartitionHandle from a global index value
+func GetPartitionHandleFromVal(val []byte) (*kv.PartitionHandle, error) {


Why not just use DecodeHandleInIndexValue function?

Defined2014 · 2024-11-02T07:35:30Z

pkg/ddl/partition.go

@@ -2277,7 +2248,7 @@ func (w *worker) onDropTablePartition(jobCtx *jobContext, job *model.Job) (ver i
 		// Since the previous state do not use the dropping partitions,
 		// we can now actually remove them, allowing to write into the overlapping range
 		// of the higher range partition or LIST default partition.
-		physicalTableIDs = updateDroppingPartitionInfo(tblInfo, partNames)
+		_ = updateDroppingPartitionInfo(tblInfo, partNames)


Suggested change

_ = updateDroppingPartitionInfo(tblInfo, partNames)

updateDroppingPartitionInfo(tblInfo, partNames)

Defined2014 · 2024-11-02T07:35:36Z

pkg/ddl/partition.go

@@ -2266,7 +2237,7 @@ func (w *worker) onDropTablePartition(jobCtx *jobContext, job *model.Job) (ver i
 		// Reason, see https://github.com/pingcap/tidb/issues/55888
 		// Only mark the partitions as to be dropped, so they are not used, but not yet removed.
 		originalDefs := tblInfo.Partition.Definitions
-		physicalTableIDs = updateDroppingPartitionInfo(tblInfo, partNames)
+		_ = updateDroppingPartitionInfo(tblInfo, partNames)


Suggested change

_ = updateDroppingPartitionInfo(tblInfo, partNames)

updateDroppingPartitionInfo(tblInfo, partNames)

mjonss added 19 commits May 31, 2024 16:52

Initial test with two domains for different SchemaVersions

cfa9bde

Added a test for AddPartition

311e71f

Updated test

4236768

Merge remote-tracking branch 'pingcap/master' into multi-domain-parti…

2154259

…tion-tests

Moved tests to a new file

b4388a7

Stabilized tests

7d4dd7e

Merge remote-tracking branch 'pingcap/master' into multi-domain-parti…

8adc5ac

…tion-tests

Updated with GLOBAL IndexOption and failpoint instead of hooks

849825d

Renamed variables

607118d

WIP test now with drop partition, needs more work, but passes

d7217ed

Updated tests

1ea37d7

Added test to see how drop unique index behaves

f51b542

Merge remote-tracking branch 'pingcap/master' into multi-domain-parti…

61f7668

…tion-tests

Updated test

d0e5b10

Refactor test and added utility function to skip on disabled failpoints

cd1f500

Merge branch 'multi-domain-partition-tests' into truncate-partition-g…

dc709d4

…lobal-index

Cleaned up non-needed tests

7e36c59

Added test case for pingcap#55819

10aac89

Delete entries during INSERT if TRUNCATE PARTITIONING is in DeleteReorg

234a636

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/invalid-title size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 3, 2024

mjonss changed the title ~~Truncate partition global index~~ *: Truncate partition with global index improvement Sep 3, 2024

ti-chi-bot bot removed the do-not-merge/invalid-title label Sep 3, 2024

mjonss added 3 commits September 3, 2024 15:55

Linting and more tests

4542f06

Updated tests and linting

6a119d7

Linting

389dd63

More tests

d162004

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2024

mjonss added 2 commits October 27, 2024 22:24

Merge remote-tracking branch 'pingcap/master' into truncate-partition…

81ff60f

…-global-index

Updated comments, and removed optimization for non-global index in tr…

fad0824

…uncate partition

mjonss added 3 commits October 30, 2024 00:14

Merge remote-tracking branch 'pingcap/master' into truncate-partition…

02401b1

…-global-index

Merge remote-tracking branch 'pingcap/master' into truncate-partition…

973f2e1

…-global-index

Merge remote-tracking branch 'pingcap/master' into truncate-partition…

7ecf919

…-global-index

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 30, 2024

mjonss added 3 commits October 30, 2024 11:26

Cleaned up unused function.

8863b8a

Fixed tests and optimized/simplified global index overwrite.

512511b

Merge remote-tracking branch 'pingcap/master' into truncate-partition…

8e09c8a

…-global-index

mjonss removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2024

mjonss added 2 commits October 30, 2024 17:01

Cleanup from non-needed change.

7c29728

Cleaunup of non-used function

2d6d7b2

Updated tests, including adding a test for Modify Column anomaly.

ba3df60

fixdb requested a review from winoros October 30, 2024 22:05

mjonss added 3 commits October 31, 2024 01:20

Added test

4391a67

Updated test, to be fixed in follow up PR pingcap#56029

8470761

Merge remote-tracking branch 'pingcap/master' into truncate-partition…

ace2802

…-global-index

winoros approved these changes Nov 1, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 1, 2024

Defined2014 reviewed Nov 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Truncate partition with global index improvement #55831

*: Truncate partition with global index improvement #55831

mjonss commented Sep 3, 2024 •

edited

Loading

tiprow bot commented Sep 3, 2024

codecov bot commented Sep 3, 2024 •

edited

Loading

mjonss commented Oct 28, 2024

tiprow bot commented Oct 28, 2024

mjonss commented Oct 30, 2024

tiprow bot commented Oct 30, 2024

purelind commented Nov 1, 2024

tiprow bot commented Nov 1, 2024

winoros left a comment

ti-chi-bot bot commented Nov 1, 2024

ti-chi-bot bot commented Nov 1, 2024

Defined2014 Nov 2, 2024

Defined2014 Nov 2, 2024

Defined2014 Nov 2, 2024

	_ = updateDroppingPartitionInfo(tblInfo, partNames)
	updateDroppingPartitionInfo(tblInfo, partNames)

*: Truncate partition with global index improvement #55831

Are you sure you want to change the base?

*: Truncate partition with global index improvement #55831

Conversation

mjonss commented Sep 3, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Sep 3, 2024

codecov bot commented Sep 3, 2024 • edited Loading

Codecov Report

mjonss commented Oct 28, 2024

tiprow bot commented Oct 28, 2024

mjonss commented Oct 30, 2024

tiprow bot commented Oct 30, 2024

purelind commented Nov 1, 2024

tiprow bot commented Nov 1, 2024

winoros left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 1, 2024

ti-chi-bot bot commented Nov 1, 2024

[LGTM Timeline notifier]

Defined2014 Nov 2, 2024

Choose a reason for hiding this comment

Defined2014 Nov 2, 2024

Choose a reason for hiding this comment

Defined2014 Nov 2, 2024

Choose a reason for hiding this comment

mjonss commented Sep 3, 2024 •

edited

Loading

codecov bot commented Sep 3, 2024 •

edited

Loading