ddl: Fix potential data lost of `alter_partition_by` #8337

JaySon-Huang · 2023-11-08T10:39:25Z

What problem does this PR solve?

Issue Number: close #8206

Problem Summary:

Introduced by #7822

When executing alter table xxx partition by ... to turn a non-partition table into partition table, there could be a chance that tiflash see a non-partition table turn into a partition table (using the same table_id). But it would be skipped by the previous implementation

tiflash/dbms/src/TiDB/Schema/SchemaBuilder.cpp

Lines 407 to 423 in b092db0

    
           template <typename Getter, typename NameMapper> 
        
           void SchemaBuilder<Getter, NameMapper>::applyPartitionDiff( 
        
               const TiDB::DBInfoPtr & db_info, 
        
               const TableInfoPtr & table_info, 
        
               const ManageableStoragePtr & storage) 
        
           { 
        
               const auto & orig_table_info = storage->getTableInfo(); 
        
               if (!orig_table_info.isLogicalPartitionTable()) 
        
               { 
        
                   LOG_ERROR( 
        
                       log, 
        
                       "old table in TiFlash not partition table {} with database_id={}, table_id={}", 
        
                       name_mapper.debugCanonicalName(*db_info, orig_table_info), 
        
                       db_info->id, 
        
                       orig_table_info.id); 
        
                   return; 
        
               }

And then tiflash mistaken drop the old table along with all its partitions, but actually those partition are now attached to a new logical table. This leads to data lost after `alter table xxx partition ...

What is changed and how it works?

Use tidb_isolation_read_engines instead of hint in test cases
Allow turning a non-partition table into partition table
When applying SchemaDiff for alter partition, we first create the new table and override the partition id mapping before dropping the old table

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

JaySon-Huang · 2023-11-08T10:55:08Z

/run-all-tests

JaySon-Huang · 2023-11-08T11:52:16Z

/hold

https://ci.pingcap.net/blue/organizations/jenkins/tiflash-ghpr-integration-tests/detail/tiflash-ghpr-integration-tests/15008/pipeline/115/

[2023-11-08T11:35:49.082Z] fullstack-test2/ddl/alter_partition_by.test: Running
[2023-11-08T11:36:15.550Z]   File: fullstack-test2/ddl/alter_partition_by.test
[2023-11-08T11:36:15.550Z]   Error line: 180
[2023-11-08T11:36:15.550Z]   Error: set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t2 partition (p0);
[2023-11-08T11:36:15.550Z]   Result:
[2023-11-08T11:36:15.550Z]     count(*)
[2023-11-08T11:36:15.550Z]     0
[2023-11-08T11:36:15.550Z]   Expected:
[2023-11-08T11:36:15.550Z]     +----------+
[2023-11-08T11:36:15.550Z]     | count(*) |
[2023-11-08T11:36:15.550Z]     +----------+
[2023-11-08T11:36:15.550Z]     |        5 |
[2023-11-08T11:36:15.550Z]     +----------+

JaySon-Huang · 2023-11-08T12:06:31Z

/run-all-tests

JaySon-Huang · 2023-11-08T14:26:47Z

/run-all-tests

ti-chi-bot · 2023-11-09T03:07:59Z

[LGTM Timeline notifier]

Timeline:

2023-11-08 14:11:41.556037286 +0000 UTC m=+3653499.143147416: ☑️ agreed by Lloyd-Pottiger.
2023-11-09 03:07:58.730113346 +0000 UTC m=+3700076.317223492: ☑️ agreed by hongyunyan.

Lloyd-Pottiger · 2023-11-09T03:55:26Z

/run-all-tests

JaySon-Huang · 2023-11-09T06:14:02Z

/run-integration-test

JaySon-Huang · 2023-11-09T13:12:22Z

/run-all-tests

tests/fullstack-test2/ddl/alter_partition_by.test

JaySon-Huang · 2023-11-09T14:37:51Z

/run-all-tests

JaySon-Huang · 2023-11-09T15:52:54Z

/run-all-tests

Signed-off-by: JaySon-Huang <[email protected]>

JaySon-Huang · 2023-11-10T03:11:50Z

/run-all-tests

hongyunyan · 2023-11-10T03:17:16Z

dbms/src/TiDB/Schema/SchemaBuilder.cpp

@@ -252,9 +252,14 @@ void SchemaBuilder<Getter, NameMapper>::applyDiff(const SchemaDiff & diff)
        }
        else
        {
-            /// The new non-partitioned table will have a new id
+            // Create the new table.
+            // If the new table is a partition table, this will also overwrite


It may be better to seperate the logical for ActionAlterTablePartitioning and ActionRemovePartitioning

For ActionRemovePartitioning, it will

Receive a schema diff, and add the new non-partition table as a table_id in partition.adding_definitions. -- TiFlash should add the new table id to mapping and handle the apply snapshot

Then receive a schema diff to make the new non-partition table as a normal table and remove the old partition-table

So it is the same logic as ActionAlterTablePartitioning in tiflash

hongyunyan · 2023-11-10T03:51:02Z

dbms/src/TiDB/Schema/SchemaBuilder.cpp

+    auto new_db_info = getter.getDatabase(database_id);
+    applyCreateStorageInstance(new_db_info, table_info);
+
+    for (const auto & part_def : table_info->partition.definitions)


when do applyPartitionDiff(new_db_info, table_info, storage), we also will emplacePartitionTableId based on the definitions, why do we also add it here?

I get what you mean, let me check it

Actually we create the Storage instance by table_info in line 402, then try to execute applyPartitionDiff. Then nothing must change between the table_info and storage.table_info. So calling applyPartitionDiff is redundant.
Then this function is simple the same as applyCreateTable. So I have remove the function applyPartitionAlter

tiflash/dbms/src/TiDB/Schema/SchemaBuilder.cpp

Lines 250 to 262 in 01a1860

case SchemaActionType::ActionRemovePartitioning:

{

if (diff.table_id == diff.old_table_id)

{

/// Only internal additions of new partitions

applyPartitionDiff(diff.schema_id, diff.table_id);

}

else

{

// Create the new table.

// If the new table is a partition table, this will also overwrite

// the partition id mapping to the new logical table

applyCreateTable(diff.schema_id, diff.table_id);

JaySon-Huang · 2023-11-10T05:53:28Z

/run-all-tests

ti-chi-bot · 2023-11-10T06:02:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hongyunyan, Lloyd-Pottiger

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~~ [Lloyd-Pottiger,hongyunyan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

JaySon-Huang · 2023-11-10T06:44:39Z

/hold cancel

ti-chi-bot · 2023-11-10T06:46:22Z

In response to a cherrypick label: new pull request created to branch release-7.1: #8355.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2023-11-10T06:47:00Z

In response to a cherrypick label: failed to apply #8337 on top of branch "release-7.5":

failed to git commit: exit status 1

ti-chi-bot bot added needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. release-note-none Denotes a PR that doesn't merit a release note. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. labels Nov 8, 2023

JaySon-Huang requested a review from hongyunyan November 8, 2023 10:39

ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 8, 2023

JaySon-Huang requested review from JinheLin and Lloyd-Pottiger November 8, 2023 10:39

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2023

Lloyd-Pottiger approved these changes Nov 8, 2023

View reviewed changes

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

hongyunyan approved these changes Nov 9, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 9, 2023

JaySon-Huang added 3 commits November 9, 2023 19:46

Use tidb_isolation_read_engine instead of hint

1da01d6

workaround

a82c452

Try fix the alter partition DDL

b76fe2e

JaySon-Huang force-pushed the fix_unstable_it branch from da25c78 to b76fe2e Compare November 9, 2023 11:47

Reduce fetching table schema from TiKV repeatedly

dbde257

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 Nov 9, 2023

Fix logging

4178513

JaySon-Huang force-pushed the fix_unstable_it branch from 7ce6aea to 4178513 Compare November 9, 2023 12:52

Add comments

adf8aa1

JaySon-Huang requested review from mjonss and hongyunyan November 9, 2023 13:12

JaySon-Huang commented Nov 9, 2023

View reviewed changes

tests/fullstack-test2/ddl/alter_partition_by.test Outdated Show resolved Hide resolved

remove fake workaround

c3d08a7

Merge branch 'master' into fix_unstable_it

16651bb

JaySon-Huang changed the title ~~tests: Fix unstable test of alter_partition_by~~ tests: Fix potential data lost of alter_partition_by Nov 9, 2023

JaySon-Huang changed the title ~~tests: Fix potential data lost of alter_partition_by~~ ddl: Fix potential data lost of alter_partition_by Nov 9, 2023

JaySon-Huang mentioned this pull request Nov 9, 2023

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

Merged

12 tasks

Refine logging

a8ac79b

Signed-off-by: JaySon-Huang <[email protected]>

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 Nov 10, 2023

hongyunyan reviewed Nov 10, 2023

View reviewed changes

Replace applyPartitionAlter by applyCreateTable

01a1860

hongyunyan approved these changes Nov 10, 2023

View reviewed changes

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2023

ti-chi-bot bot merged commit 27de3d3 into pingcap:master Nov 10, 2023

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

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

Closed

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Nov 10, 2023

This is an automated cherry-pick of pingcap#8337

db22a85

Signed-off-by: ti-chi-bot <[email protected]>

JaySon-Huang deleted the fix_unstable_it branch November 10, 2023 07:21

JaySon-Huang removed the needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. label Nov 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: Fix potential data lost of `alter_partition_by` #8337

ddl: Fix potential data lost of `alter_partition_by` #8337

JaySon-Huang commented Nov 8, 2023 •

edited

Loading

JaySon-Huang commented Nov 8, 2023

JaySon-Huang commented Nov 8, 2023 •

edited

Loading

JaySon-Huang commented Nov 8, 2023

JaySon-Huang commented Nov 8, 2023

ti-chi-bot bot commented Nov 9, 2023

Lloyd-Pottiger commented Nov 9, 2023

JaySon-Huang commented Nov 9, 2023

JaySon-Huang commented Nov 9, 2023

JaySon-Huang commented Nov 9, 2023

JaySon-Huang commented Nov 9, 2023

JaySon-Huang commented Nov 10, 2023

hongyunyan Nov 10, 2023

JaySon-Huang Nov 10, 2023

hongyunyan Nov 10, 2023

This comment was marked as off-topic.

JaySon-Huang Nov 10, 2023

JaySon-Huang Nov 10, 2023

JaySon-Huang commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

JaySon-Huang commented Nov 10, 2023

ti-chi-bot commented Nov 10, 2023

ti-chi-bot commented Nov 10, 2023

	template <typename Getter, typename NameMapper>
	void SchemaBuilder<Getter, NameMapper>::applyPartitionDiff(
	const TiDB::DBInfoPtr & db_info,
	const TableInfoPtr & table_info,
	const ManageableStoragePtr & storage)
	{
	const auto & orig_table_info = storage->getTableInfo();
	if (!orig_table_info.isLogicalPartitionTable())
	{
	LOG_ERROR(
	log,
	"old table in TiFlash not partition table {} with database_id={}, table_id={}",
	name_mapper.debugCanonicalName(*db_info, orig_table_info),
	db_info->id,
	orig_table_info.id);
	return;
	}

	case SchemaActionType::ActionRemovePartitioning:
	{
	if (diff.table_id == diff.old_table_id)
	{
	/// Only internal additions of new partitions
	applyPartitionDiff(diff.schema_id, diff.table_id);
	}
	else
	{
	// Create the new table.
	// If the new table is a partition table, this will also overwrite
	// the partition id mapping to the new logical table
	applyCreateTable(diff.schema_id, diff.table_id);

ddl: Fix potential data lost of alter_partition_by #8337

ddl: Fix potential data lost of alter_partition_by #8337

Conversation

JaySon-Huang commented Nov 8, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

JaySon-Huang commented Nov 8, 2023

JaySon-Huang commented Nov 8, 2023 • edited Loading

JaySon-Huang commented Nov 8, 2023

JaySon-Huang commented Nov 8, 2023

ti-chi-bot bot commented Nov 9, 2023

[LGTM Timeline notifier]

Lloyd-Pottiger commented Nov 9, 2023

JaySon-Huang commented Nov 9, 2023

JaySon-Huang commented Nov 9, 2023

JaySon-Huang commented Nov 9, 2023

JaySon-Huang commented Nov 9, 2023

JaySon-Huang commented Nov 10, 2023

hongyunyan Nov 10, 2023

Choose a reason for hiding this comment

JaySon-Huang Nov 10, 2023

Choose a reason for hiding this comment

hongyunyan Nov 10, 2023

Choose a reason for hiding this comment

This comment was marked as off-topic.

JaySon-Huang Nov 10, 2023

Choose a reason for hiding this comment

JaySon-Huang Nov 10, 2023

Choose a reason for hiding this comment

JaySon-Huang commented Nov 10, 2023

ti-chi-bot bot commented Nov 10, 2023

JaySon-Huang commented Nov 10, 2023

ti-chi-bot commented Nov 10, 2023

ti-chi-bot commented Nov 10, 2023

ddl: Fix potential data lost of `alter_partition_by` #8337

ddl: Fix potential data lost of `alter_partition_by` #8337

JaySon-Huang commented Nov 8, 2023 •

edited

Loading

JaySon-Huang commented Nov 8, 2023 •

edited

Loading