From 717405f5e69774f8fd580c72564c768fc1c81da6 Mon Sep 17 00:00:00 2001 From: JaySon Date: Thu, 16 Nov 2023 10:38:16 +0800 Subject: [PATCH] ddl: Fix failure on executing exchange partition(release-6.5) (#8374) close pingcap/tiflash#8372 --- dbms/src/Encryption/RateLimiter.cpp | 2 +- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 75 +++++++------- dbms/src/TiDB/Schema/SchemaBuilder.h | 2 +- .../ddl/alter_exchange_partition.test | 98 +++++++++++++++++++ 4 files changed, 142 insertions(+), 35 deletions(-) diff --git a/dbms/src/Encryption/RateLimiter.cpp b/dbms/src/Encryption/RateLimiter.cpp index cc77984facd..ab45c84ea6c 100644 --- a/dbms/src/Encryption/RateLimiter.cpp +++ b/dbms/src/Encryption/RateLimiter.cpp @@ -712,7 +712,7 @@ IOLimitTuner::TuneResult IOLimitTuner::tune() const auto msg = fmt::format("limiter {} write {} read {}", limiterCount(), writeLimiterCount(), readLimiterCount()); if (limiterCount() < 2) { - LOG_INFO(log, "{} NOT need to tune.", msg); + LOG_TRACE(log, "{} NOT need to tune.", msg); return {0, 0, false, 0, 0, false}; } LOG_INFO(log, "{} need to tune.", msg); diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index f19b2bcbc06..d69b6c49a98 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -595,11 +595,11 @@ void SchemaBuilder::applyPartitionDiff(const TiDB::DBInfoPtr throw TiFlashException(fmt::format("miss table in TiFlash {}", table_id), Errors::DDL::MissingTable); } - applyPartitionDiff(db_info, table_info, storage); + applyPartitionDiff(db_info, table_info, storage, /*drop_part_if_not_exist*/ true); } template -void SchemaBuilder::applyPartitionDiff(const TiDB::DBInfoPtr & db_info, const TableInfoPtr & table_info, const ManageableStoragePtr & storage) +void SchemaBuilder::applyPartitionDiff(const TiDB::DBInfoPtr & db_info, const TableInfoPtr & table_info, const ManageableStoragePtr & storage, bool drop_part_if_not_exist) { const auto & orig_table_info = storage->getTableInfo(); if (!orig_table_info.isLogicalPartitionTable()) @@ -641,13 +641,17 @@ void SchemaBuilder::applyPartitionDiff(const TiDB::DBInfoPtr updated_table_info.partition = table_info->partition; /// Apply changes to physical tables. - for (const auto & orig_def : orig_defs) + if (drop_part_if_not_exist) { - if (new_part_id_set.count(orig_def.id) == 0) + for (const auto & orig_def : orig_defs) { - applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), orig_def.id); + if (new_part_id_set.count(orig_def.id) == 0) + { + applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), orig_def.id); + } } } + for (const auto & new_def : new_defs) { if (orig_part_id_set.count(new_def.id) == 0) @@ -777,7 +781,7 @@ void SchemaBuilder::applyExchangeTablePartition(const Schema diff.table_id); /// Exchange table partition is used for ddl: - /// alter table partition_table exchange partition partition_name with table non_partition_table + /// `ALTER TABLE partition_table EXCHANGE PARTITION partition_name WITH TABLE non_partition_table` /// It involves three table/partition: partition_table, partition_name and non_partition_table /// The table id/schema id for the 3 table/partition are stored in SchemaDiff as follows: /// Table_id in diff is the partition id of which will be exchanged, @@ -788,19 +792,23 @@ void SchemaBuilder::applyExchangeTablePartition(const Schema GET_METRIC(tiflash_schema_internal_ddl_count, type_exchange_partition).Increment(); if (diff.affected_opts.empty()) throw Exception("Incorrect schema diff, no affected_opts for alter table exchange partition schema diff", ErrorCodes::DDL_ERROR); - auto npt_db_info = getter.getDatabase(diff.schema_id); + const auto npt_database_id = diff.schema_id; + const auto pt_database_id = diff.affected_opts[0].schema_id; + auto npt_db_info = getter.getDatabase(npt_database_id); if (npt_db_info == nullptr) throw TiFlashException(fmt::format("miss database: {}", diff.schema_id), Errors::DDL::StaleSchema); - auto pt_db_info = getter.getDatabase(diff.affected_opts[0].schema_id); + auto pt_db_info = getter.getDatabase(pt_database_id); if (pt_db_info == nullptr) throw TiFlashException(fmt::format("miss database: {}", diff.affected_opts[0].schema_id), Errors::DDL::StaleSchema); - auto npt_table_id = diff.old_table_id; - auto pt_partition_id = diff.table_id; - auto pt_table_info = diff.affected_opts[0].table_id; + const auto npt_table_id = diff.old_table_id; + const auto pt_partition_id = diff.table_id; + const auto pt_table_id = diff.affected_opts[0].table_id; + + LOG_INFO(log, "Execute exchange partition begin, npt_table_id={} npt_database_id={} pt_table_id={} pt_partition_id={} pt_database_id={}", npt_table_id, npt_database_id, pt_table_id, pt_partition_id, pt_database_id); /// step 1 change the mete data of partition table - auto table_info = getter.getTableInfo(pt_db_info->id, pt_table_info); + auto table_info = getter.getTableInfo(pt_db_info->id, pt_table_id); // latest partition table info from TiKV if (table_info == nullptr) - throw TiFlashException(fmt::format("miss table in TiKV : {}", pt_table_info), Errors::DDL::StaleSchema); + throw TiFlashException(fmt::format("miss table in TiKV : pt_table_id={}", pt_table_id), Errors::DDL::StaleSchema); auto & tmt_context = context.getTMTContext(); auto storage = tmt_context.getStorages().get(table_info->id); if (storage == nullptr) @@ -808,28 +816,25 @@ void SchemaBuilder::applyExchangeTablePartition(const Schema fmt::format("miss table in TiFlash : {}", name_mapper.debugCanonicalName(*pt_db_info, *table_info)), Errors::DDL::MissingTable); - LOG_INFO(log, "Exchange partition for table {}", name_mapper.debugCanonicalName(*pt_db_info, *table_info)); - auto orig_table_info = storage->getTableInfo(); - orig_table_info.partition = table_info->partition; - { - auto alter_lock = storage->lockForAlter(getThreadName()); - storage->alterFromTiDB( - alter_lock, - AlterCommands{}, - name_mapper.mapDatabaseName(*pt_db_info), - orig_table_info, - name_mapper, - context); - } + // Apply the new partitions to the logical table. + /// - create the new physical tables according to the new partition definitions + /// - persist the new table info to disk + // The latest table info could be the table info after `EXCHANGE PARTITION` is executed + // on TiDB. So we need to apply and also create the physical tables of new ids appear in + // the partition list. Because we can not get a table schema by its physical_table_id + // once it became a partition. + // But this method will skip dropping partition id that is not exist in the new table_info, + // because the physical table could be changed into a normal table without dropping. + applyPartitionDiff(pt_db_info, table_info, storage, /*drop_part_if_not_exist*/ false); FAIL_POINT_TRIGGER_EXCEPTION(FailPoints::exception_after_step_1_in_exchange_partition); /// step 2 change non partition table to a partition of the partition table storage = tmt_context.getStorages().get(npt_table_id); if (storage == nullptr) - throw TiFlashException(fmt::format("miss table in TiFlash : {}", name_mapper.debugCanonicalName(*npt_db_info, *table_info)), + throw TiFlashException(fmt::format("miss table in TiFlash, npt_table_id={} : {}", npt_table_id, name_mapper.debugCanonicalName(*npt_db_info, *table_info)), Errors::DDL::MissingTable); - orig_table_info = storage->getTableInfo(); - orig_table_info.belonging_table_id = pt_table_info; + auto orig_table_info = storage->getTableInfo(); + orig_table_info.belonging_table_id = pt_table_id; orig_table_info.is_partition_table = true; /// partition does not have explicit name, so use default name here orig_table_info.name = name_mapper.mapTableName(orig_table_info); @@ -852,11 +857,14 @@ void SchemaBuilder::applyExchangeTablePartition(const Schema /// step 3 change partition of the partition table to non partition table table_info = getter.getTableInfo(npt_db_info->id, pt_partition_id); if (table_info == nullptr) - throw TiFlashException(fmt::format("miss table in TiKV : {}", pt_partition_id), Errors::DDL::StaleSchema); - storage = tmt_context.getStorages().get(table_info->id); + { + LOG_WARNING(log, "Execute exchange partition, the table info of partition can not get from TiKV, npt_database_id={} partition_id={}", npt_database_id, pt_partition_id); + throw TiFlashException(fmt::format("miss partition table in TiKV, may have been dropped, physical_table_id={}", pt_partition_id), Errors::DDL::StaleSchema); + } + storage = tmt_context.getStorages().get(pt_partition_id); if (storage == nullptr) throw TiFlashException( - fmt::format("miss table in TiFlash : {}", name_mapper.debugCanonicalName(*pt_db_info, *table_info)), + fmt::format("miss partition table in TiFlash, physical_table_id={}", pt_partition_id), Errors::DDL::MissingTable); orig_table_info = storage->getTableInfo(); orig_table_info.belonging_table_id = DB::InvalidTableID; @@ -877,6 +885,7 @@ void SchemaBuilder::applyExchangeTablePartition(const Schema if (npt_db_info->id != pt_db_info->id) applyRenamePhysicalTable(npt_db_info, orig_table_info, storage); FAIL_POINT_TRIGGER_EXCEPTION(FailPoints::exception_after_step_3_in_exchange_partition); + LOG_INFO(log, "Execute exchange partition done, npt_table_id={} npt_database_id={} pt_table_id={} pt_partition_id={} pt_database_id={}", npt_table_id, npt_database_id, pt_table_id, pt_partition_id, pt_database_id); } template @@ -1347,7 +1356,7 @@ void SchemaBuilder::syncAllSchema() if (table->isLogicalPartitionTable()) { /// Apply partition diff if needed. - applyPartitionDiff(db, table, storage); + applyPartitionDiff(db, table, storage, /*drop_part_if_not_exist*/ true); } /// Rename if needed. applyRenameLogicalTable(db, table, storage); diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.h b/dbms/src/TiDB/Schema/SchemaBuilder.h index 50c28dc33a9..8a84efd7afd 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.h +++ b/dbms/src/TiDB/Schema/SchemaBuilder.h @@ -70,7 +70,7 @@ struct SchemaBuilder void applyPartitionDiff(const TiDB::DBInfoPtr & db_info, TableID table_id); - void applyPartitionDiff(const TiDB::DBInfoPtr & db_info, const TiDB::TableInfoPtr & table_info, const ManageableStoragePtr & storage); + void applyPartitionDiff(const TiDB::DBInfoPtr & db_info, const TiDB::TableInfoPtr & table_info, const ManageableStoragePtr & storage, bool drop_part_if_not_exist); void applyAlterTable(const TiDB::DBInfoPtr & db_info, TableID table_id); diff --git a/tests/fullstack-test2/ddl/alter_exchange_partition.test b/tests/fullstack-test2/ddl/alter_exchange_partition.test index 50e950a797a..0d5558f1d84 100644 --- a/tests/fullstack-test2/ddl/alter_exchange_partition.test +++ b/tests/fullstack-test2/ddl/alter_exchange_partition.test @@ -267,6 +267,104 @@ mysql> set session tidb_isolation_read_engines='tiflash'; select * from test_new mysql> alter table test.e drop column c1; >> DBGInvoke __refresh_schemas() +# cleanup +mysql> drop table if exists test.e; +mysql> drop table if exists test.e2; +mysql> drop table if exists test_new.e2; +mysql> drop database if exists test_new; + +# case 11, create non-partition table and execute exchagne partition immediately +mysql> create table test.e(id INT NOT NULL,fname VARCHAR(30),lname VARCHAR(30)) PARTITION BY RANGE (id) ( PARTITION p0 VALUES LESS THAN (50),PARTITION p1 VALUES LESS THAN (100),PARTITION p2 VALUES LESS THAN (150), PARTITION p3 VALUES LESS THAN (MAXVALUE)); +mysql> insert into test.e values (1, 'a', 'b'),(108, 'a', 'b'); +# sync the partition table to tiflash +>> DBGInvoke __refresh_schemas() + +mysql> create table test.e2(id int not null,fname varchar(30),lname varchar(30)); +mysql> insert into test.e2 values (2, 'a', 'b'); +mysql> set @@tidb_enable_exchange_partition=1; alter table test.e exchange partition p0 with table test.e2 + +mysql> alter table test.e set tiflash replica 1; +mysql> alter table test.e2 set tiflash replica 1; +func> wait_table test e e2 +>> DBGInvoke __refresh_schemas() +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.e order by id; ++-----+-------+-------+ +| id | fname | lname | ++-----+-------+-------+ +| 2 | a | b | +| 108 | a | b | ++-----+-------+-------+ +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.e2 order by id; ++-----+-------+-------+ +| id | fname | lname | ++-----+-------+-------+ +| 1 | a | b | ++-----+-------+-------+ + +# ensure the swap out table is not mark as tombstone +>> DBGInvoke __enable_schema_sync_service('true') +>> DBGInvoke __gc_schemas(18446744073709551615) +>> DBGInvoke __enable_schema_sync_service('false') +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.e order by id; ++-----+-------+-------+ +| id | fname | lname | ++-----+-------+-------+ +| 2 | a | b | +| 108 | a | b | ++-----+-------+-------+ +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.e2 order by id; ++-----+-------+-------+ +| id | fname | lname | ++-----+-------+-------+ +| 1 | a | b | ++-----+-------+-------+ + +# case 12, create partition table, non-partition table and execute exchagne partition immediately +mysql> drop table if exists test.e +mysql> drop table if exists test.e2 +mysql> create table test.e(id INT NOT NULL,fname VARCHAR(30),lname VARCHAR(30)) PARTITION BY RANGE (id) ( PARTITION p0 VALUES LESS THAN (50),PARTITION p1 VALUES LESS THAN (100),PARTITION p2 VALUES LESS THAN (150), PARTITION p3 VALUES LESS THAN (MAXVALUE)); +mysql> insert into test.e values (1, 'a', 'b'),(108, 'a', 'b'); +mysql> create table test.e2(id int not null,fname varchar(30),lname varchar(30)); +mysql> insert into test.e2 values (2, 'a', 'b'); +mysql> set @@tidb_enable_exchange_partition=1; alter table test.e exchange partition p0 with table test.e2 + +mysql> alter table test.e set tiflash replica 1; +mysql> alter table test.e2 set tiflash replica 1; +func> wait_table test e e2 +# tiflash the final result +>> DBGInvoke __refresh_schemas() +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.e order by id; ++-----+-------+-------+ +| id | fname | lname | ++-----+-------+-------+ +| 2 | a | b | +| 108 | a | b | ++-----+-------+-------+ +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.e2 order by id; ++-----+-------+-------+ +| id | fname | lname | ++-----+-------+-------+ +| 1 | a | b | ++-----+-------+-------+ +# ensure the swap out table is not mark as tombstone +>> DBGInvoke __enable_schema_sync_service('true') +>> DBGInvoke __gc_schemas(18446744073709551615) +>> DBGInvoke __enable_schema_sync_service('false') +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.e order by id; ++-----+-------+-------+ +| id | fname | lname | ++-----+-------+-------+ +| 2 | a | b | +| 108 | a | b | ++-----+-------+-------+ +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.e2 order by id; ++-----+-------+-------+ +| id | fname | lname | ++-----+-------+-------+ +| 1 | a | b | ++-----+-------+-------+ + +# cleanup mysql> drop table if exists test.e; mysql> drop table if exists test.e2; mysql> drop table if exists test_new.e2;