From 34459589ce307d70e41b1ce721929d6a4db1c9e3 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 15 Nov 2023 15:44:38 +0800 Subject: [PATCH 1/3] Add fullstack test case --- .../ddl/alter_exchange_partition.test | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) 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; From 6e9d65fce0650b089cb192a73f91d68c56c78ce4 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 15 Nov 2023 15:46:35 +0800 Subject: [PATCH 2/3] Fix bug in exchange partition --- .../Storages/Transaction/SchemaBuilder.cpp | 75 +++++++++++-------- dbms/src/Storages/Transaction/SchemaBuilder.h | 2 +- 2 files changed, 43 insertions(+), 34 deletions(-) diff --git a/dbms/src/Storages/Transaction/SchemaBuilder.cpp b/dbms/src/Storages/Transaction/SchemaBuilder.cpp index 381570a9bd3..c8473a5f291 100644 --- a/dbms/src/Storages/Transaction/SchemaBuilder.cpp +++ b/dbms/src/Storages/Transaction/SchemaBuilder.cpp @@ -581,11 +581,11 @@ void SchemaBuilder::applyPartitionDiff(TiDB::DBInfoPtr db_in 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(TiDB::DBInfoPtr db_info, TableInfoPtr table_info, ManageableStoragePtr storage) +void SchemaBuilder::applyPartitionDiff(TiDB::DBInfoPtr db_info, TableInfoPtr table_info, ManageableStoragePtr storage, bool drop_part_if_not_exist) { const auto & orig_table_info = storage->getTableInfo(); if (!orig_table_info.isLogicalPartitionTable()) @@ -627,13 +627,17 @@ void SchemaBuilder::applyPartitionDiff(TiDB::DBInfoPtr db_in 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) @@ -744,7 +748,7 @@ template void SchemaBuilder::applyExchangeTablePartition(const SchemaDiff & diff) { /// 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, @@ -755,19 +759,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_FMT_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) @@ -775,28 +783,25 @@ void SchemaBuilder::applyExchangeTablePartition(const Schema fmt::format("miss table in TiFlash : {}", name_mapper.debugCanonicalName(*pt_db_info, *table_info)), Errors::DDL::MissingTable); - LOG_FMT_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); @@ -819,11 +824,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_FMT_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; @@ -844,6 +852,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_FMT_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 @@ -1294,7 +1303,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/Storages/Transaction/SchemaBuilder.h b/dbms/src/Storages/Transaction/SchemaBuilder.h index 01c0cafa755..cd853f4d414 100644 --- a/dbms/src/Storages/Transaction/SchemaBuilder.h +++ b/dbms/src/Storages/Transaction/SchemaBuilder.h @@ -70,7 +70,7 @@ struct SchemaBuilder void applyPartitionDiff(TiDB::DBInfoPtr db_info, TableID table_id); - void applyPartitionDiff(TiDB::DBInfoPtr db_info, TiDB::TableInfoPtr table_info, ManageableStoragePtr storage); + void applyPartitionDiff(TiDB::DBInfoPtr db_info, TiDB::TableInfoPtr table_info, ManageableStoragePtr storage, bool drop_part_if_not_exist); void applyAlterTable(TiDB::DBInfoPtr db_info, TableID table_id); From 99d0a366f91a0e7a30696c4552534bc6b87f6f9b Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 15 Nov 2023 15:47:18 +0800 Subject: [PATCH 3/3] Suppress a verbose logging --- dbms/src/Encryption/RateLimiter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Encryption/RateLimiter.cpp b/dbms/src/Encryption/RateLimiter.cpp index b68e6d4c777..ae360d40b95 100644 --- a/dbms/src/Encryption/RateLimiter.cpp +++ b/dbms/src/Encryption/RateLimiter.cpp @@ -741,7 +741,7 @@ IOLimitTuner::TuneResult IOLimitTuner::tune() const auto msg = fmt::format("limiter {} write {} read {}", limiterCount(), writeLimiterCount(), readLimiterCount()); if (limiterCount() < 2) { - LOG_FMT_INFO(log, "{} NOT need to tune.", msg); + LOG_FMT_TRACE(log, "{} NOT need to tune.", msg); return {0, 0, false, 0, 0, false}; } LOG_FMT_INFO(log, "{} need to tune.", msg);