Added support for new DDL actions for remove and add table partitioning #7822

mjonss · 2023-07-19T14:35:03Z

What problem does this PR solve?

Issue Number: close #7823
Problem Summary:
ALTER TABLE t REMOVE PARTITIONING
and
ALTER TABLE t PARTITION BY ...
introduces new DDL Actions, which is not supported by TiFlash.
Handling those is the same as for other partitioning management DDLs, so it is easy to implement.

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

None

Lloyd-Pottiger · 2023-07-21T01:42:00Z

/run-all-tests

tests/fullstack-test2/ddl/alter_partition_by.test

tests/fullstack-test2/ddl/remove_partitioning.test

hongyunyan · 2023-07-25T08:35:54Z

dbms/src/TiDB/Schema/SchemaBuilder.cpp

@@ -219,6 +219,24 @@ void SchemaBuilder<Getter, NameMapper>::applyDiff(const SchemaDiff & diff)
        applyPartitionDiff(diff.schema_id, diff.table_id);
        break;
    }
+    case SchemaActionType::ActionAlterTablePartitioning:
+    {
+        if (diff.table_id == diff.old_table_id)


I'm curious about that when diff.table_id = diff.old_table_id and when diff.table_id != diff.old_table_id, could you please give an example here?

It is depending on the stage change, only the last (StateDeleteReorganization -> StatePublic) is the table ID changed to the new one.

Example (added custom logs):
tidb.log

[2023/08/01 23:11:59.568 +02:00] [WARN] [ddl_worker.go:1431] [updateSchemaVersion] [category=ddl] [TableId=96] [SchemaState="delete only"] [2023/08/01 23:11:59.637 +02:00] [WARN] [ddl_worker.go:1431] [updateSchemaVersion] [category=ddl] [TableId=96] [SchemaState="delete only"] [2023/08/01 23:11:59.702 +02:00] [WARN] [ddl_worker.go:1431] [updateSchemaVersion] [category=ddl] [TableId=96] [SchemaState="write only"] [2023/08/01 23:11:59.798 +02:00] [WARN] [ddl_worker.go:1431] [updateSchemaVersion] [category=ddl] [TableId=96] [SchemaState="write reorganization"] [2023/08/01 23:11:59.865 +02:00] [WARN] [ddl_worker.go:1431] [updateSchemaVersion] [category=ddl] [TableId=96] [SchemaState="delete reorganization"] [2023/08/01 23:11:59.865 +02:00] [WARN] [ddl_worker.go:1441] [StateDeleteReorganization] [category=ddl] [TableId=105] [OldTableID=96]

tiflash.log

[2023/08/01 23:11:59.818 +02:00] [WARN] [SchemaBuilder.cpp:238] ["ActionRemovePartitioning table_id 96 old 96"] [source="keyspace=4294967295"] [thread_id=60] [2023/08/01 23:11:59.819 +02:00] [WARN] [SchemaBuilder.cpp:238] ["ActionRemovePartitioning table_id 96 old 96"] [source="keyspace=4294967295"] [thread_id=60] [2023/08/01 23:11:59.820 +02:00] [WARN] [SchemaBuilder.cpp:238] ["ActionRemovePartitioning table_id 96 old 96"] [source="keyspace=4294967295"] [thread_id=60] [2023/08/01 23:11:59.821 +02:00] [WARN] [SchemaBuilder.cpp:238] ["ActionRemovePartitioning table_id 96 old 96"] [source="keyspace=4294967295"] [thread_id=60] [2023/08/01 23:12:55.086 +02:00] [WARN] [SchemaBuilder.cpp:238] ["ActionRemovePartitioning table_id 105 old 96"] [source="keyspace=4294967295"] [thread_id=61]

So in the first three stages, does the partition.definitions changes?

In the first (StateNone->StateDeleteOnly), it adds the new partitions to TableInfo.Partition.AddingDefinitions and the removed ones to TableInfo.Partition.DroppingDefinitions.

In the second (StateDeleteOnly->StateWriteOnly), it only waits for the partitions to be available in TiFlash (if the table should have any TiFlash replicas).

In the third (StateWriteOnly->StateWriteReorganization), no changes or checks.

In the forth (StateWriteReorganization->StateDeleteReorganization), no changes (but does all the data copying and recreating the indexes).

In the fifth (StateDeleteReorganization->StatePublic), it depends on the actual command (reorganize, remove partitioning or alter table partition by):

Reorganize, it will modify the TableInfo.Partition.Definitions by removing the DroppingDefinitions and add the AddingDefinitions and then reset AddingDefinitions and DroppingDefinitions.

Remove partitioning, it will change the tableID to the one used for AddingDefinitions. Really drop the old partitioned table metadata, change the TableInfo by setting the table id to the single AddingDefinitions table id, and remove TableInfo.Partition, and then create the table (metadata) with the updated TableInfo.

Alter Table Partition By, it will change the TableID and replace the TableInfo.Partition with the new partitioning type, expression and definitions. Also by first drop the table (metadata), change the TableInfo.Partition and table id, and the (re)create the table with the updated TableInfo.

Notice that the data cleanup is done later in an async way, and that the old table data also needs to be accessible during the schemaversion the DDL got during transitioning to StatePublic, since there may be clients in the schemaversion of StateDeleteReorganization that still need to read and (double) write the data.

After some additional testing I figured out that I missed to drop the table (metadata) in TiFlash as well, not just create a new table :)

hongyunyan

LGTM

JaySon-Huang · 2023-08-22T07:52:07Z

/run-all-tests

JaySon-Huang · 2023-08-22T09:39:32Z

/run-all-tests

JaySon-Huang · 2023-08-25T10:21:25Z

/run-all-tests

mjonss · 2023-08-29T12:40:23Z

/test-unit-test

mjonss · 2023-08-29T13:07:27Z

/run-all-tests

mjonss · 2023-09-01T08:19:14Z

The failing integration test fullstack-test2/ddl/rename_pk.test also fails locally, without any code changes. Is this test also failing in other PRs? @hongyunyan any ideas on how to fix it?

mjonss · 2023-09-01T09:10:07Z

/run-all-tests

JaySon-Huang · 2023-09-04T14:11:47Z

/run-all-tests

JaySon-Huang · 2023-09-05T02:50:55Z

blocked by pingcap/tidb#46645

[2023-09-04T14:20:07.289Z] fullstack-test2/ddl/multi_alter_with_write.test: OK [0.033 s]
[2023-09-04T14:20:07.289Z] fullstack-test2/ddl/remove_partitioning.test: Running
[2023-09-04T14:20:11.493Z] fullstack-test2/ddl/remove_partitioning.test: OK [4.101 s]
[2023-09-04T14:20:11.493Z] fullstack-test2/ddl/rename_pk.test: Running
[2023-09-04T14:20:13.380Z]   File: fullstack-test2/ddl/rename_pk.test
[2023-09-04T14:20:13.380Z]   Error line: 23
[2023-09-04T14:20:13.380Z]   Error: set session tidb_isolation_read_engines='tiflash'; select * from test.t order by pk;
[2023-09-04T14:20:13.380Z]   Result:
[2023-09-04T14:20:13.380Z]     ERROR 1815 (HY000) at line 1: Internal : Can't find a proper physical plan for this query
[2023-09-04T14:20:13.380Z]   Expected:
[2023-09-04T14:20:13.380Z]     +----+
[2023-09-04T14:20:13.380Z]     | pk |
[2023-09-04T14:20:13.380Z]     +----+
[2023-09-04T14:20:13.380Z]     |  1 |
[2023-09-04T14:20:13.380Z]     |  2 |
[2023-09-04T14:20:13.380Z]     +----+

bb7133 · 2023-09-05T10:41:31Z

blocked by pingcap/tidb#46645

@JaySon-Huang @mjonss Does it mean that some PR(which got merged recently) broke the CI of tiflash?

JaySon-Huang · 2023-09-11T07:51:28Z

tests/fullstack-test2/ddl/remove_partitioning.test

+
+func> wait_table test t
+
+mysql> set @@global.tidb_allow_mpp = 0;


Suggested change

mysql> set @@global.tidb_allow_mpp = 0;

Is tidb_allow_mpp = 0 necessary for your test cases? It will make the following tests fail.

I've pushed a commit to remove it for a try.

JaySon-Huang · 2023-09-11T07:55:02Z

/run-all-tests

JaySon-Huang · 2023-09-11T08:27:22Z

/run-unit-test

JaySon-Huang · 2023-09-11T08:28:05Z

/run-integration-test

ti-chi-bot · 2023-09-11T08:28:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hongyunyan, JaySon-Huang

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JaySon-Huang,hongyunyan]

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 · 2023-09-11T08:28:21Z

[LGTM Timeline notifier]

Timeline:

2023-08-07 05:53:48.041197625 +0000 UTC m=+683111.983546155: ☑️ agreed by hongyunyan.
2023-09-11 08:28:19.934525353 +0000 UTC m=+257881.859081748: ☑️ agreed by JaySon-Huang.

JaySon-Huang · 2023-09-11T08:32:57Z

waiting for https://ci.pingcap.net/blue/organizations/jenkins/tiflash-ghpr-integration-tests/detail/tiflash-ghpr-integration-tests/14439/pipeline/128

JaySon-Huang · 2023-09-11T08:46:50Z

/run-integration-test

mjonss · 2023-09-11T08:54:41Z

/run-integration-test

Added support for new DDL actions for remove and add table partitioning

f0ba7ea

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/needs-linked-issue labels Jul 19, 2023

Updated remove and add partitioning logic

43d7778

JaySon-Huang requested review from JaySon-Huang and hongyunyan July 20, 2023 03:27

hongyunyan reviewed Jul 25, 2023

View reviewed changes

Fixed bug with non-cleaned table definitions

9ef312a

hongyunyan approved these changes Aug 7, 2023

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Aug 7, 2023

mjonss mentioned this pull request Jul 4, 2023

Support ALTER TABLE t REMOVE PARTITIONING pingcap/tidb#42616

Closed

14 tasks

mjonss and others added 2 commits August 18, 2023 15:26

Merge branch 'master' into remove-and-add-partitioning

78f03e8

Merge branch 'master' into remove-and-add-partitioning

abf77ed

Merge branch 'master' into remove-and-add-partitioning

4a1b412

mjonss and others added 2 commits August 25, 2023 11:54

Merge branch 'master' into remove-and-add-partitioning

853ee18

Merge branch 'master' into remove-and-add-partitioning

9d8df4e

mjonss and others added 3 commits August 28, 2023 11:56

Merge branch 'master' into remove-and-add-partitioning

2ebdfe2

Merge branch 'master' into remove-and-add-partitioning

cd52346

Added test for non-partitioned -> partitioned

a7e381e

ti-chi-bot 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 Aug 29, 2023

Merge branch 'master' into remove-and-add-partitioning

1903458

Merge branch 'master' into remove-and-add-partitioning

b4c1d03

ti-chi-bot bot added the do-not-merge/needs-triage-completed label Sep 7, 2023

Merge branch 'master' into remove-and-add-partitioning

efe59de

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Sep 8, 2023

JaySon-Huang reviewed Sep 11, 2023

View reviewed changes

Remove tidb global settings

4c7ae94

JaySon-Huang approved these changes Sep 11, 2023

View reviewed changes

ti-chi-bot bot added the lgtm label Sep 11, 2023

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Sep 11, 2023

ti-chi-bot bot merged commit 1539364 into pingcap:master Sep 11, 2023

mjonss deleted the remove-and-add-partitioning branch September 11, 2023 09:14

mjonss mentioned this pull request Sep 14, 2023

Added REMOVE PARTITIONING and ALTER TABLE t PARTITION BY pingcap/docs#13167

Merged

15 tasks

qiancai mentioned this pull request Sep 21, 2023

Added REMOVE PARTITIONING and ALTER TABLE t PARTITION BY pingcap/docs-cn#15069

Merged

15 tasks

This was referenced Nov 9, 2023

ddl: Fix potential data lost of alter_partition_by #8337

Merged

ddl: Fix potential data lost of alter_partition_by (release-7.5) #8350

Merged

ti-chi-bot mentioned this pull request Nov 10, 2023

ddl: Fix potential data lost of alter_partition_by (#8337) #8355

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for new DDL actions for remove and add table partitioning #7822

Added support for new DDL actions for remove and add table partitioning #7822

mjonss commented Jul 19, 2023 •

edited

Loading

Lloyd-Pottiger commented Jul 21, 2023

hongyunyan Jul 25, 2023

mjonss Aug 1, 2023

hongyunyan Aug 3, 2023

mjonss Aug 3, 2023

mjonss Aug 3, 2023

hongyunyan left a comment

JaySon-Huang commented Aug 22, 2023

JaySon-Huang commented Aug 22, 2023

JaySon-Huang commented Aug 25, 2023

mjonss commented Aug 29, 2023

mjonss commented Aug 29, 2023

mjonss commented Sep 1, 2023

mjonss commented Sep 1, 2023

JaySon-Huang commented Sep 4, 2023

JaySon-Huang commented Sep 5, 2023

bb7133 commented Sep 5, 2023 •

edited

Loading

JaySon-Huang Sep 11, 2023

JaySon-Huang Sep 11, 2023 •

edited

Loading

JaySon-Huang commented Sep 11, 2023

JaySon-Huang commented Sep 11, 2023

JaySon-Huang commented Sep 11, 2023

ti-chi-bot bot commented Sep 11, 2023

ti-chi-bot bot commented Sep 11, 2023

JaySon-Huang commented Sep 11, 2023

JaySon-Huang commented Sep 11, 2023

mjonss commented Sep 11, 2023


		func> wait_table test t

		mysql> set @@global.tidb_allow_mpp = 0;

Added support for new DDL actions for remove and add table partitioning #7822

Added support for new DDL actions for remove and add table partitioning #7822

Conversation

mjonss commented Jul 19, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Lloyd-Pottiger commented Jul 21, 2023

hongyunyan Jul 25, 2023

Choose a reason for hiding this comment

mjonss Aug 1, 2023

Choose a reason for hiding this comment

hongyunyan Aug 3, 2023

Choose a reason for hiding this comment

mjonss Aug 3, 2023

Choose a reason for hiding this comment

mjonss Aug 3, 2023

Choose a reason for hiding this comment

hongyunyan left a comment

Choose a reason for hiding this comment

JaySon-Huang commented Aug 22, 2023

JaySon-Huang commented Aug 22, 2023

JaySon-Huang commented Aug 25, 2023

mjonss commented Aug 29, 2023

mjonss commented Aug 29, 2023

mjonss commented Sep 1, 2023

mjonss commented Sep 1, 2023

JaySon-Huang commented Sep 4, 2023

JaySon-Huang commented Sep 5, 2023

bb7133 commented Sep 5, 2023 • edited Loading

JaySon-Huang Sep 11, 2023

Choose a reason for hiding this comment

JaySon-Huang Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

JaySon-Huang commented Sep 11, 2023

JaySon-Huang commented Sep 11, 2023

JaySon-Huang commented Sep 11, 2023

ti-chi-bot bot commented Sep 11, 2023

ti-chi-bot bot commented Sep 11, 2023

[LGTM Timeline notifier]

JaySon-Huang commented Sep 11, 2023

JaySon-Huang commented Sep 11, 2023

mjonss commented Sep 11, 2023

mjonss commented Jul 19, 2023 •

edited

Loading

bb7133 commented Sep 5, 2023 •

edited

Loading

JaySon-Huang Sep 11, 2023 •

edited

Loading