ddl: Coalesce partition | tidb-test=pr/2111 #42476

mjonss · 2023-03-22T09:20:54Z

What problem does this PR solve?

Issue Number: close #15002

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 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-03-22T09:20:56Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

TonsnakeLin
crazycs520

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-03-22T09:20:56Z

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

mjonss · 2023-03-22T09:24:00Z

/test all

mjonss · 2023-04-04T21:45:37Z

/retest

mjonss · 2023-04-05T08:00:31Z

/retest

TonsnakeLin · 2023-04-07T08:49:08Z

ddl/ddl_api.go

@@ -3868,6 +3868,19 @@ func (d *ddl) AddTablePartitions(ctx sessionctx.Context, ident ast.Ident, spec *
 	if pi == nil {
 		return errors.Trace(dbterror.ErrPartitionMgmtOnNonpartitioned)
 	}
+	if pi.Type == model.PartitionTypeHash || pi.Type == model.PartitionTypeKey {
+		// Add partition for hash/key is actually a reorganize partition


Suggested change

// Add partition for hash/key is actually a reorganize partition

// Adding partition for hash/key is actually a reorganize partition

The command is 'ADD PARTITION', so that is what I used here, but I don't mind changing the comment if you want?

ddl/ddl_api.go

TonsnakeLin · 2023-04-07T12:20:11Z

ddl/partition.go

+	// So start by re-using the existing ones
+	// For COALESCE, it will copy the remaining ones
+	// For ADD it will copy the existing ones
+	copy(definitions, tbInfo.Partition.Definitions)


I run the sql alter table t add partition partitions 3 and alter table t coalesce partition 8, no one can get here.
What's the scenes when len(defs) != 0 ?

If you specify the partition definitions in CREATE or ADD PARTITION, you will have definitions, not just a number.

Like:

ALTER TABLE t ADD PARTITION (PARTITION pWithName1 COMMENT = 'Not really useful, but can be used for placement rules', PARTITION pWithName2);

But I don't understand the comment "So start by re-using the existing ones".
In the past, we don't copy the entire information from tbInfo.Partition.Definitions, only set Name, Comment,Policy. Why do we need copy the entire information now ?

ddl/ddl_api.go

mjonss · 2023-04-09T11:04:29Z

/retest

ddl/db_partition_test.go

ddl/partition.go

tiancaiamao

Please resolve comment from the others, rest looks good to me.

tiancaiamao · 2023-04-11T00:39:57Z

ddl/ddl_api.go

+		}
+	}
+
+	return d.ReorganizePartitions(sctx, ident, &newSpec)


So this line is all the real changes for this functionality ...

Correct, it is reusing the heavy lifting done and tested in Reorganize Partition (copy row data between partitions and recreating the indexes on each new partition).

mjonss · 2023-04-12T12:51:17Z

/retest

crazycs520

LGTM

TonsnakeLin · 2023-04-15T09:55:34Z

ddl/partition.go

+	// So start by re-using the existing ones
+	// For COALESCE, it will copy the remaining ones
+	// For ADD it will copy the existing ones
+	copy(definitions, tbInfo.Partition.Definitions)


But I don't understand the comment "So start by re-using the existing ones".
In the past, we don't copy the entire information from tbInfo.Partition.Definitions, only set Name, Comment,Policy. Why do we need copy the entire information now ?

TonsnakeLin · 2023-04-15T09:58:13Z

ddl/partition.go

+
+	// ADD PARTITION needs to add the new partitions
+	for i := offset; i < numParts; i++ {
+		if (i - offset) < uint64(len(defs)) {


Is there any case which will cover the if (i - offset) < uint64(len(defs))... code block and else code block at the same time. If no, why dont' separate the two from loop.

There should not be, since number of partitions partitions N and partition definitions (partition p1 ..., ...) are mutual exclusive.

But I rewrote it slightly to avoid copying all information, and only copying name, comment and placement rules.

TonsnakeLin · 2023-04-17T02:09:22Z

LGTM

bb7133

LGTM

bb7133 · 2023-04-17T03:41:33Z

/merge

ti-chi-bot · 2023-04-17T03:41:36Z

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

Commit hash: 2e248a3

hawkingrei · 2023-04-17T05:45:21Z

/retest

mjonss added 2 commits March 21, 2023 12:10

Implemented ALTER TABLE COALESCE PARTITION for HASH and KEY

1b0ec7c

Implemented ADD PARTITION for HASH/KEY

a6d39aa

ti-chi-bot removed the do-not-merge/needs-linked-issue label Mar 27, 2023

Updated test

2701402

mjonss mentioned this pull request Mar 29, 2023

ddl: Remove partitioning #42475

Closed

12 tasks

mjonss added 4 commits March 30, 2023 16:51

Added checks and updated errors

406388c

Extended test

4657f13

Cleanup

6c93d7a

Updated error message

c29a784

mjonss mentioned this pull request Mar 31, 2023

Support MySQL Partition management #42728

Closed

5 tasks

mjonss marked this pull request as ready for review April 3, 2023 09:53

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2023

mjonss changed the title ~~ddl: Coalesce partition~~ ddl: Coalesce partition | tidb-test=pr/2111 Apr 3, 2023

mjonss added 4 commits April 5, 2023 17:37

Keep partition options on coalesce partition

58b0495

Merge remote-tracking branch 'pingcap/master' into coalesce-partition

7dba320

Linting

e65c270

Fixed test

f120990

mjonss mentioned this pull request Apr 6, 2023

Added COALESCE PARTITION pingcap/docs#13162

Merged

15 tasks

mjonss requested review from TonsnakeLin and tiancaiamao April 6, 2023 14:05

TonsnakeLin reviewed Apr 7, 2023

View reviewed changes

mjonss mentioned this pull request Apr 7, 2023

*: Support List default partition | tidb-test=pr/2110 #41537

Merged

12 tasks

TonsnakeLin reviewed Apr 7, 2023

View reviewed changes

crazycs520 reviewed Apr 7, 2023

View reviewed changes

ddl/ddl_api.go Outdated Show resolved Hide resolved

mjonss added 2 commits April 7, 2023 17:00

Less impact on tableInfo meta, added reviewers suggestions

80b7ed2

Added error for HISTORY partition clause in HASH/KEY add partition

40ced84

crazycs520 reviewed Apr 10, 2023

View reviewed changes

ddl/db_partition_test.go Show resolved Hide resolved

crazycs520 reviewed Apr 10, 2023

View reviewed changes

ddl/db_partition_test.go Show resolved Hide resolved

crazycs520 reviewed Apr 10, 2023

View reviewed changes

ddl/partition.go Outdated Show resolved Hide resolved

tiancaiamao reviewed Apr 11, 2023

View reviewed changes

mjonss added 2 commits April 12, 2023 13:49

Fixed add hash/key partition removing previous partition options

30b34e7

Merge remote-tracking branch 'pingcap/master' into coalesce-partition

78da493

crazycs520 approved these changes Apr 12, 2023

View reviewed changes

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

TonsnakeLin reviewed Apr 15, 2023

View reviewed changes

mjonss added 2 commits April 16, 2023 02:45

Only copy name, comment and placement policy for coalesce partition

b4e6d3a

Merge remote-tracking branch 'pingcap/master' into coalesce-partition

2e248a3

TonsnakeLin approved these changes Apr 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 Apr 17, 2023

bb7133 approved these changes Apr 17, 2023

View reviewed changes

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

ti-chi-bot merged commit 4878ed3 into pingcap:master Apr 17, 2023

hawkingrei mentioned this pull request Apr 17, 2023

DATA RACE at the StatementStatsMap #43100

Open

mjonss deleted the coalesce-partition branch May 8, 2023 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: Coalesce partition | tidb-test=pr/2111 #42476

ddl: Coalesce partition | tidb-test=pr/2111 #42476

mjonss commented Mar 22, 2023 •

edited

Loading

ti-chi-bot commented Mar 22, 2023 •

edited

Loading

ti-chi-bot commented Mar 22, 2023

mjonss commented Mar 22, 2023

mjonss commented Apr 4, 2023

mjonss commented Apr 5, 2023

TonsnakeLin Apr 7, 2023

mjonss Apr 7, 2023

TonsnakeLin Apr 7, 2023

mjonss Apr 7, 2023

mjonss Apr 7, 2023

TonsnakeLin Apr 15, 2023

mjonss commented Apr 9, 2023

tiancaiamao left a comment

tiancaiamao Apr 11, 2023

mjonss Apr 12, 2023

mjonss commented Apr 12, 2023

crazycs520 left a comment

TonsnakeLin Apr 15, 2023

TonsnakeLin Apr 15, 2023

mjonss Apr 16, 2023

TonsnakeLin commented Apr 17, 2023

bb7133 left a comment

bb7133 commented Apr 17, 2023

ti-chi-bot commented Apr 17, 2023

hawkingrei commented Apr 17, 2023

	// Add partition for hash/key is actually a reorganize partition
	// Adding partition for hash/key is actually a reorganize partition

ddl: Coalesce partition | tidb-test=pr/2111 #42476

ddl: Coalesce partition | tidb-test=pr/2111 #42476

Conversation

mjonss commented Mar 22, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 22, 2023 • edited Loading

ti-chi-bot commented Mar 22, 2023

mjonss commented Mar 22, 2023

mjonss commented Apr 4, 2023

mjonss commented Apr 5, 2023

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

mjonss commented Apr 9, 2023

tiancaiamao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjonss commented Apr 12, 2023

crazycs520 left a 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

TonsnakeLin commented Apr 17, 2023

bb7133 left a comment

Choose a reason for hiding this comment

bb7133 commented Apr 17, 2023

ti-chi-bot commented Apr 17, 2023

hawkingrei commented Apr 17, 2023

mjonss commented Mar 22, 2023 •

edited

Loading

ti-chi-bot commented Mar 22, 2023 •

edited

Loading