From 1da01d64c039e8c37967e1f55e192118e7a23928 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 8 Nov 2023 18:26:37 +0800 Subject: [PATCH 01/11] Use tidb_isolation_read_engine instead of hint --- .../ddl/alter_partition_by.test | 37 ++++++++++--------- .../ddl/remove_partitioning.test | 34 ++++++++++++++--- .../ddl/reorganize_partition.test | 24 ++++++------ 3 files changed, 59 insertions(+), 36 deletions(-) diff --git a/tests/fullstack-test2/ddl/alter_partition_by.test b/tests/fullstack-test2/ddl/alter_partition_by.test index a05f2a43ce2..a9569c1332b 100644 --- a/tests/fullstack-test2/ddl/alter_partition_by.test +++ b/tests/fullstack-test2/ddl/alter_partition_by.test @@ -28,7 +28,7 @@ mysql> insert into test.t select a+1000000,a+1000000,-(a+1000000) from test.t; func> wait_table test t -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ * from test.t order by a; +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.t order by a; +---------+---------+----------+ | a | b | c | +---------+---------+----------+ @@ -56,7 +56,7 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ * from test.t order by a; │ test │ t │ └───────────────┴───────────┘ -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p0); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p0); +----------+ | count(*) | +----------+ @@ -64,7 +64,7 @@ mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition ( +----------+ mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p0); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p0); +----------+ | count(*) | +----------+ @@ -72,14 +72,14 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partitio +----------+ mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p1M); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p1M); +----------+ | count(*) | +----------+ | 8 | +----------+ -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p1M); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p1M); +----------+ | count(*) | +----------+ @@ -90,7 +90,7 @@ mysql> show warnings; mysql> alter table test.t partition by range (a) (partition p0 values less than (500000), partition p500k values less than (1000000), partition p1M values less than (2000000)); -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p0); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p0); +----------+ | count(*) | +----------+ @@ -99,7 +99,7 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partitio mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p500k); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p500k); +----------+ | count(*) | +----------+ @@ -108,14 +108,14 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partitio mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p0); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p0); +----------+ | count(*) | +----------+ | 4 | +----------+ -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p500k); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p500k); +----------+ | count(*) | +----------+ @@ -124,14 +124,14 @@ mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition ( mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p1M); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p1M); +----------+ | count(*) | +----------+ | 8 | +----------+ -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p1M); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p1M); +----------+ | count(*) | +----------+ @@ -144,7 +144,7 @@ mysql> alter table test.t2 set tiflash replica 1; mysql> insert into test.t2 select * from test.t; func> wait_table test t2 -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t2]) */ * from test.t2 order by a; +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.t2 order by a; +---------+---------+----------+ | a | b | c | +---------+---------+----------+ @@ -168,6 +168,7 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t2]) */ * from test.t2 order by a; mysql> drop table test.t; +# check the t2 is created in TiFlash >> select tidb_database,tidb_name from system.tables where tidb_database = 'test' and tidb_name = 't2' and is_tombstone = 0 ┌─tidb_database─┬─tidb_name─┐ │ test │ t2 │ @@ -176,7 +177,7 @@ mysql> drop table test.t; mysql> alter table test.t2 partition by hash (a) partitions 3; mysql> analyze table test.t2; -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t2]) */ count(*) from test.t2 partition (p0); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t2 partition (p0); +----------+ | count(*) | +----------+ @@ -185,7 +186,7 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t2]) */ count(*) from test.t2 partit mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t2]) */ count(*) from test.t2 partition (p1); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t2 partition (p1); +----------+ | count(*) | +----------+ @@ -194,14 +195,14 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t2]) */ count(*) from test.t2 partit mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIKV[t2]) */ count(*) from test.t2 partition (p0); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t2 partition (p0); +----------+ | count(*) | +----------+ | 5 | +----------+ -mysql> select /*+ READ_FROM_STORAGE(TIKV[t2]) */ count(*) from test.t2 partition (p1); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t2 partition (p1); +----------+ | count(*) | +----------+ @@ -211,14 +212,14 @@ mysql> select /*+ READ_FROM_STORAGE(TIKV[t2]) */ count(*) from test.t2 partition mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIKV[t2]) */ count(*) from test.t2 partition (p2); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t2 partition (p2); +----------+ | count(*) | +----------+ | 5 | +----------+ -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t2]) */ count(*) from test.t2 partition (p2); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t2 partition (p2); +----------+ | count(*) | +----------+ diff --git a/tests/fullstack-test2/ddl/remove_partitioning.test b/tests/fullstack-test2/ddl/remove_partitioning.test index 2f8f9574be8..2ce14aa9cc4 100644 --- a/tests/fullstack-test2/ddl/remove_partitioning.test +++ b/tests/fullstack-test2/ddl/remove_partitioning.test @@ -28,13 +28,35 @@ mysql> insert into test.t select a+1000000,a+1000000,-(a+1000000) from test.t; func> wait_table test t +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.t; ++---------+---------+----------+ +| a | b | c | ++---------+---------+----------+ +| 1 | 1 | -1 | +| 2 | 2 | -2 | +| 3 | 3 | -3 | +| 4 | 4 | -4 | +| 500001 | 500001 | -500001 | +| 500002 | 500002 | -500002 | +| 500003 | 500003 | -500003 | +| 500004 | 500004 | -500004 | +| 1000001 | 1000001 | -1000001 | +| 1000002 | 1000002 | -1000002 | +| 1000003 | 1000003 | -1000003 | +| 1000004 | 1000004 | -1000004 | +| 1500001 | 1500001 | -1500001 | +| 1500002 | 1500002 | -1500002 | +| 1500003 | 1500003 | -1500003 | +| 1500004 | 1500004 | -1500004 | ++---------+---------+----------+ + # check table info in tiflash >> select tidb_database,tidb_name from system.tables where tidb_database = 'test' and tidb_name = 't' and is_tombstone = 0 ┌─tidb_database─┬─tidb_name─┐ │ test │ t │ └───────────────┴───────────┘ -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p0); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p0); +----------+ | count(*) | +----------+ @@ -42,7 +64,7 @@ mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition ( +----------+ mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p0); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p0); +----------+ | count(*) | +----------+ @@ -50,14 +72,14 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partitio +----------+ mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p1M); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p1M); +----------+ | count(*) | +----------+ | 8 | +----------+ -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p1M); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p1M); +----------+ | count(*) | +----------+ @@ -68,7 +90,7 @@ mysql> show warnings; mysql> alter table test.t remove partitioning; -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t; +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t; +----------+ | count(*) | +----------+ @@ -77,7 +99,7 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t; mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t; +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t; +----------+ | count(*) | +----------+ diff --git a/tests/fullstack-test2/ddl/reorganize_partition.test b/tests/fullstack-test2/ddl/reorganize_partition.test index 971a4ce8341..9453bc373e3 100644 --- a/tests/fullstack-test2/ddl/reorganize_partition.test +++ b/tests/fullstack-test2/ddl/reorganize_partition.test @@ -34,7 +34,7 @@ func> wait_table test t │ test │ t │ └───────────────┴───────────┘ -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p0); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p0); +----------+ | count(*) | +----------+ @@ -42,7 +42,7 @@ mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition ( +----------+ mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p0); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p0); +----------+ | count(*) | +----------+ @@ -50,14 +50,14 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partitio +----------+ mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p1M); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p1M); +----------+ | count(*) | +----------+ | 8 | +----------+ -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p1M); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p1M); +----------+ | count(*) | +----------+ @@ -68,7 +68,7 @@ mysql> show warnings; mysql> alter table test.t reorganize partition p0 INTO (partition p0 values less than (500000), partition p500k values less than (1000000)); -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p0); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p0); +----------+ | count(*) | +----------+ @@ -77,7 +77,7 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partitio mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p500k); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p500k); +----------+ | count(*) | +----------+ @@ -86,14 +86,14 @@ mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partitio mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p0); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p0); +----------+ | count(*) | +----------+ | 4 | +----------+ -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p500k); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p500k); +----------+ | count(*) | +----------+ @@ -102,14 +102,14 @@ mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition ( mysql> show warnings; -mysql> select /*+ READ_FROM_STORAGE(TIKV[t]) */ count(*) from test.t partition (p1M); +mysql> set session tidb_isolation_read_engines='tikv'; select count(*) from test.t partition (p1M); +----------+ | count(*) | +----------+ | 8 | +----------+ -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[t]) */ count(*) from test.t partition (p1M); +mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t partition (p1M); +----------+ | count(*) | +----------+ @@ -147,14 +147,14 @@ mysql> alter table test.t1 reorganize partition p1 INTO (partition p1 values les # make write cmd take effect >> DBGInvoke __disable_fail_point(pause_before_apply_raft_cmd) -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[test.t1]) */ * from test.t1 partition (p1); +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.t1 partition (p1); +----+------+----+ | id | name | c | +----+------+----+ | 60 | cba |NULL| +----+------+----+ -mysql> select /*+ READ_FROM_STORAGE(TIFLASH[test.t1]) */ * from test.t1 partition (p2); +mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.t1 partition (p2); +----+------+----+ | id | name | c | +----+------+----+ From a82c45241eb2160caabe58703e0217f24fadd456 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 8 Nov 2023 20:05:41 +0800 Subject: [PATCH 02/11] workaround --- tests/fullstack-test2/ddl/alter_partition_by.test | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/fullstack-test2/ddl/alter_partition_by.test b/tests/fullstack-test2/ddl/alter_partition_by.test index a9569c1332b..51339106290 100644 --- a/tests/fullstack-test2/ddl/alter_partition_by.test +++ b/tests/fullstack-test2/ddl/alter_partition_by.test @@ -143,6 +143,7 @@ mysql> create table test.t2 (a int primary key, b varchar(255), c int, key (b), mysql> alter table test.t2 set tiflash replica 1; mysql> insert into test.t2 select * from test.t; func> wait_table test t2 +mysql> drop table test.t; mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.t2 order by a; +---------+---------+----------+ @@ -166,8 +167,6 @@ mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.t2 | 1500004 | 1500004 | -1500004 | +---------+---------+----------+ -mysql> drop table test.t; - # check the t2 is created in TiFlash >> select tidb_database,tidb_name from system.tables where tidb_database = 'test' and tidb_name = 't2' and is_tombstone = 0 ┌─tidb_database─┬─tidb_name─┐ @@ -177,6 +176,9 @@ mysql> drop table test.t; mysql> alter table test.t2 partition by hash (a) partitions 3; mysql> analyze table test.t2; +#TODO this is just a workaround, the tidb return before waitting tiflash replica available for the new partitions +SLEEP 10 + mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t2 partition (p0); +----------+ | count(*) | From b76fe2ed912843f1e14221821685b7da97358b89 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 9 Nov 2023 19:46:07 +0800 Subject: [PATCH 03/11] Try fix the alter partition DDL --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 81 +++++++++++++++++--------- dbms/src/TiDB/Schema/SchemaGetter.cpp | 1 + dbms/src/TiDB/Schema/TiDB.cpp | 12 ++-- dbms/src/TiDB/Schema/TiDB.h | 2 + 4 files changed, 64 insertions(+), 32 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index db65b9b8416..e1556523ae3 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -253,8 +253,14 @@ void SchemaBuilder::applyDiff(const SchemaDiff & diff) else { /// The new non-partitioned table will have a new id - applyDropTable(diff.schema_id, diff.old_table_id); + // 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); + // try to renew the partition info for the new table + applyPartitionDiff(diff.schema_id, diff.table_id); + // drop the old table + applyDropTable(diff.schema_id, diff.old_table_id); } break; } @@ -410,75 +416,79 @@ void SchemaBuilder::applyPartitionDiff( const TableInfoPtr & table_info, const ManageableStoragePtr & storage) { - const auto & orig_table_info = storage->getTableInfo(); - if (!orig_table_info.isLogicalPartitionTable()) + const auto & local_table_info = storage->getTableInfo(); + // ALTER TABLE t PARTITION BY ... may turn a non-partition table into partition table + if (!local_table_info.isLogicalPartitionTable()) { - LOG_ERROR( + LOG_INFO( log, - "old table in TiFlash not partition table {} with database_id={}, table_id={}", - name_mapper.debugCanonicalName(*db_info, orig_table_info), + "Altering non-partition table to be a partition table {} with database_id={}, table_id={}", + name_mapper.debugCanonicalName(*db_info, local_table_info), db_info->id, - orig_table_info.id); - return; + local_table_info.id); } - const auto & orig_defs = orig_table_info.partition.definitions; + const auto & local_defs = local_table_info.partition.definitions; const auto & new_defs = table_info->partition.definitions; - std::unordered_set orig_part_id_set, new_part_id_set; - std::vector orig_part_ids, new_part_ids; - std::for_each(orig_defs.begin(), orig_defs.end(), [&orig_part_id_set, &orig_part_ids](const auto & def) { - orig_part_id_set.emplace(def.id); - orig_part_ids.emplace_back(std::to_string(def.id)); + std::unordered_set local_part_id_set, new_part_id_set; + std::for_each(local_defs.begin(), local_defs.end(), [&local_part_id_set](const auto & def) { + local_part_id_set.emplace(def.id); }); - std::for_each(new_defs.begin(), new_defs.end(), [&new_part_id_set, &new_part_ids](const auto & def) { + std::for_each(new_defs.begin(), new_defs.end(), [&new_part_id_set](const auto & def) { new_part_id_set.emplace(def.id); - new_part_ids.emplace_back(std::to_string(def.id)); }); - auto orig_part_ids_str = boost::algorithm::join(orig_part_ids, ", "); - auto new_part_ids_str = boost::algorithm::join(new_part_ids, ", "); - LOG_INFO( log, "Applying partition changes {} with database_id={}, table_id={}, old: {}, new: {}", name_mapper.debugCanonicalName(*db_info, *table_info), db_info->id, table_info->id, - orig_part_ids_str, - new_part_ids_str); + local_part_id_set, + new_part_id_set); - if (orig_part_id_set == new_part_id_set) + if (local_part_id_set == new_part_id_set) { LOG_INFO( log, - "No partition changes {} with database_id={}, table_id={}", + "No partition changes, paritions_size={} {} with database_id={}, table_id={}", name_mapper.debugCanonicalName(*db_info, *table_info), + new_part_id_set.size(), db_info->id, table_info->id); return; } - auto updated_table_info = orig_table_info; + // Copy the local table info and update fileds on the copy + auto updated_table_info = local_table_info; + updated_table_info.is_partition_table = true; + updated_table_info.belonging_table_id = table_info->belonging_table_id; updated_table_info.partition = table_info->partition; /// Apply changes to physical tables. - for (const auto & orig_def : orig_defs) + for (const auto & local_def : local_defs) { - if (new_part_id_set.count(orig_def.id) == 0) + if (!new_part_id_set.contains(local_def.id)) { - applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), orig_def.id); + applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), local_def.id); } } for (const auto & new_def : new_defs) { - if (orig_part_id_set.count(new_def.id) == 0) + if (!local_part_id_set.contains(new_def.id)) { table_id_map.emplacePartitionTableID(new_def.id, updated_table_info.id); } } + if (table_info->partition.new_table_id != DB::InvalidTableID) + { + // TODO: maybe we should create the new_logical_table here? But we don't know + // the database_id of new_logical_table + } + auto alter_lock = storage->lockForAlter(getThreadNameAndID()); storage->alterSchemaChange( alter_lock, @@ -1041,6 +1051,21 @@ void SchemaBuilder::applyDropTable(DatabaseID database_id, T { for (const auto & part_def : table_info.partition.definitions) { + if (TableID latest_logical_table_id = table_id_map.findTableIDInPartitionMap(part_def.id); + latest_logical_table_id == -1 || latest_logical_table_id != table_info.id) + { + // The partition is managed by another logical table now, skip dropping this partition + // when dropping the old logical table + LOG_INFO( + log, + "The partition is not managed by current logical table, skip, partition_table_id={} " + "new_logical_table_id={} current_table_id={}", + part_def.id, + latest_logical_table_id, + table_info.id); + continue; + } + applyDropPhysicalTable(name_mapper.mapDatabaseName(database_id, keyspace_id), part_def.id); } } diff --git a/dbms/src/TiDB/Schema/SchemaGetter.cpp b/dbms/src/TiDB/Schema/SchemaGetter.cpp index 29ec04fdc46..47ee084341c 100644 --- a/dbms/src/TiDB/Schema/SchemaGetter.cpp +++ b/dbms/src/TiDB/Schema/SchemaGetter.cpp @@ -251,6 +251,7 @@ std::optional SchemaGetter::getSchemaDiff(Int64 ver) LOG_WARNING(log, "The schema diff for version {}, key {} is empty.", ver, key); return std::nullopt; } + LOG_DEBUG(log, "Get SchemaDiff from TiKV: {}", data); // TODO: turn it into trace level SchemaDiff diff; diff.deserialize(data); return diff; diff --git a/dbms/src/TiDB/Schema/TiDB.cpp b/dbms/src/TiDB/Schema/TiDB.cpp index a7224af78ae..d36308e0b90 100644 --- a/dbms/src/TiDB/Schema/TiDB.cpp +++ b/dbms/src/TiDB/Schema/TiDB.cpp @@ -540,6 +540,10 @@ try part_id_set.emplace(definition.id); } + /// Treat `adding_definitions` and `dropping_definitions` as the normal `definitions` + /// in TiFlash. Because TiFlash need to create the physical IStorage instance + /// to handle the data on those partitions during DDL. + auto add_defs_json = json->getArray("adding_definitions"); if (!add_defs_json.isNull()) { @@ -569,6 +573,8 @@ try } num = json->getValue("num"); + + new_table_id = json->getValue("new_table_id"); } catch (const Poco::Exception & e) { @@ -851,8 +857,6 @@ TableInfo::TableInfo(const String & table_info_json, KeyspaceID keyspace_id_) String TableInfo::serialize() const try { - std::stringstream buf; - Poco::JSON::Object::Ptr json = new Poco::JSON::Object(); json->set("id", id); json->set("keyspace_id", keyspace_id); @@ -867,8 +871,8 @@ try auto col_obj = col_info.getJSONObject(); cols_arr->add(col_obj); } - json->set("cols", cols_arr); + Poco::JSON::Array::Ptr index_arr = new Poco::JSON::Array(); for (const auto & index_info : index_infos) { @@ -904,8 +908,8 @@ try json->set("tiflash_replica", replica_info.getJSONObject()); + std::stringstream buf; json->stringify(buf); - return buf.str(); } catch (const Poco::Exception & e) diff --git a/dbms/src/TiDB/Schema/TiDB.h b/dbms/src/TiDB/Schema/TiDB.h index 26af887824f..7813bcd459a 100644 --- a/dbms/src/TiDB/Schema/TiDB.h +++ b/dbms/src/TiDB/Schema/TiDB.h @@ -266,6 +266,8 @@ struct PartitionInfo bool enable = false; std::vector definitions; UInt64 num = 0; + + TableID new_table_id = DB::InvalidTableID; }; struct DBInfo From dbde2572d7594e76a5d90cee10a1f38479601e50 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 9 Nov 2023 20:23:35 +0800 Subject: [PATCH 04/11] Reduce fetching table schema from TiKV repeatedly --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 60 ++++++++++++++++++++++---- dbms/src/TiDB/Schema/SchemaBuilder.h | 3 +- dbms/src/TiDB/Schema/TiDB.cpp | 5 ++- 3 files changed, 58 insertions(+), 10 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index e1556523ae3..cf769181b6c 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -252,14 +252,13 @@ void SchemaBuilder::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 the partition id mapping to the - // new logical table. - applyCreateTable(diff.schema_id, diff.table_id); - // try to renew the partition info for the new table - applyPartitionDiff(diff.schema_id, diff.table_id); - // drop the old table + // 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 and renew the + // partition info. + applyPartitionAlter(diff.schema_id, diff.table_id); + // Drop the old table. if the previous partitions of the old table are + // not mapping to the old logical table now, they will not be removed. applyDropTable(diff.schema_id, diff.old_table_id); } break; @@ -379,6 +378,51 @@ void SchemaBuilder::applySetTiFlashReplica(DatabaseID databa } } +template +void SchemaBuilder::applyPartitionAlter(DatabaseID database_id, TableID table_id) +{ + auto table_info = getter.getTableInfo(database_id, table_id); + if (table_info == nullptr) // the database maybe dropped + { + LOG_DEBUG(log, "table is not exist in TiKV, may have been dropped, table_id={}", table_id); + return; + } + + table_id_map.emplaceTableID(table_id, database_id); + LOG_DEBUG(log, "register table to table_id_map, database_id={} table_id={}", database_id, table_id); + + if (!table_info->isLogicalPartitionTable()) + { + return; + } + + // If table is partition table, we will create the logical table here. + // Because we get the table_info, so we can ensure new_db_info will not be nullptr. + auto new_db_info = getter.getDatabase(database_id); + applyCreatePhysicalTable(new_db_info, table_info); + + for (const auto & part_def : table_info->partition.definitions) + { + LOG_DEBUG( + log, + "register table to table_id_map for partition table, logical_table_id={} physical_table_id={}", + table_id, + part_def.id); + table_id_map.emplacePartitionTableID(part_def.id, table_id); + } + + auto & tmt_context = context.getTMTContext(); + auto storage = tmt_context.getStorages().get(keyspace_id, table_info->id); + if (storage == nullptr) + { + LOG_ERROR(log, "table is not exist in TiFlash, table_id={}", table_id); + return; + } + + // Try to renew the partition info for the new table + applyPartitionDiff(new_db_info, table_info, storage); +} + template void SchemaBuilder::applyPartitionDiff(DatabaseID database_id, TableID table_id) { diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.h b/dbms/src/TiDB/Schema/SchemaBuilder.h index 780521b5775..fd4be82d5cf 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.h +++ b/dbms/src/TiDB/Schema/SchemaBuilder.h @@ -206,8 +206,9 @@ struct SchemaBuilder /// Parameter schema_name should be mapped. void applyDropPhysicalTable(const String & db_name, TableID table_id); - void applyPartitionDiff(DatabaseID database_id, TableID table_id); + void applyPartitionAlter(DatabaseID database_id, TableID table_id); + void applyPartitionDiff(DatabaseID database_id, TableID table_id); void applyPartitionDiff( const TiDB::DBInfoPtr & db_info, const TiDB::TableInfoPtr & table_info, diff --git a/dbms/src/TiDB/Schema/TiDB.cpp b/dbms/src/TiDB/Schema/TiDB.cpp index d36308e0b90..20dc695a1f2 100644 --- a/dbms/src/TiDB/Schema/TiDB.cpp +++ b/dbms/src/TiDB/Schema/TiDB.cpp @@ -574,7 +574,10 @@ try num = json->getValue("num"); - new_table_id = json->getValue("new_table_id"); + if (json->has("new_table_id")) + { + new_table_id = json->getValue("new_table_id"); + } } catch (const Poco::Exception & e) { From 417851397de2d939a4ab1f26babbfba34580093b Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 9 Nov 2023 20:50:49 +0800 Subject: [PATCH 05/11] Fix logging --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 6 +++--- dbms/src/TiDB/Schema/SchemaGetter.cpp | 25 +++++++++++++------------ 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index cf769181b6c..328769f3894 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -844,7 +844,7 @@ void SchemaBuilder::applyDropSchema(const String & db_name) auto db = context.tryGetDatabase(db_name); if (db == nullptr) { - LOG_INFO(log, "Database {} does not exists", db_name); + LOG_INFO(log, "Database does not exist, db_name={}", db_name); return; } @@ -1046,7 +1046,7 @@ void SchemaBuilder::applyDropPhysicalTable(const String & db auto storage = tmt_context.getStorages().get(keyspace_id, table_id); if (storage == nullptr) { - LOG_DEBUG(log, "table {} does not exist.", table_id); + LOG_DEBUG(log, "table does not exist, table_id={}", table_id); return; } GET_METRIC(tiflash_schema_internal_ddl_count, type_drop_table).Increment(); @@ -1087,7 +1087,7 @@ void SchemaBuilder::applyDropTable(DatabaseID database_id, T auto * storage = tmt_context.getStorages().get(keyspace_id, table_id).get(); if (storage == nullptr) { - LOG_DEBUG(log, "table {} does not exist.", table_id); + LOG_DEBUG(log, "table does not exist, table_id={}", table_id); return; } const auto & table_info = storage->getTableInfo(); diff --git a/dbms/src/TiDB/Schema/SchemaGetter.cpp b/dbms/src/TiDB/Schema/SchemaGetter.cpp index 47ee084341c..5e170dd7fd6 100644 --- a/dbms/src/TiDB/Schema/SchemaGetter.cpp +++ b/dbms/src/TiDB/Schema/SchemaGetter.cpp @@ -248,10 +248,10 @@ std::optional SchemaGetter::getSchemaDiff(Int64 ver) String data = TxnStructure::get(snap, key); if (data.empty()) { - LOG_WARNING(log, "The schema diff for version {}, key {} is empty.", ver, key); + LOG_WARNING(log, "The schema diff is empty, schema_version={} key={}", ver, key); return std::nullopt; } - LOG_DEBUG(log, "Get SchemaDiff from TiKV: {}", data); // TODO: turn it into trace level + LOG_TRACE(log, "Get SchemaDiff from TiKV, schema_version={} data={}", ver, data); SchemaDiff diff; diff.deserialize(data); return diff; @@ -275,7 +275,7 @@ TiDB::DBInfoPtr SchemaGetter::getDatabase(DatabaseID db_id) if (json.empty()) return nullptr; - LOG_DEBUG(log, "Get DB Info from TiKV : " + json); + LOG_DEBUG(log, "Get DB Info from TiKV: {}", json); auto db_info = std::make_shared(json, keyspace_id); return db_info; } @@ -285,25 +285,26 @@ TiDB::TableInfoPtr SchemaGetter::getTableInfo(DatabaseID db_id, TableID table_id String db_key = getDBKey(db_id); if (!checkDBExists(db_key)) { - LOG_ERROR(log, "The database {} does not exist.", db_id); + LOG_ERROR(log, "The database does not exist, database_id={}", db_id); return nullptr; } String table_key = getTableKey(table_id); String table_info_json = TxnStructure::hGet(snap, db_key, table_key); if (table_info_json.empty()) { - LOG_WARNING(log, "The table {} is dropped in TiKV, try to get the latest table_info", table_id); + LOG_WARNING(log, "The table is dropped in TiKV, try to get the latest table_info, table_id={}", table_id); table_info_json = TxnStructure::mvccGet(snap, db_key, table_key); if (table_info_json.empty()) { LOG_ERROR( log, - "The table {} is dropped in TiKV, and the latest table_info is still empty, it should by gc", + "The table is dropped in TiKV, and the latest table_info is still empty, it should be GCed, " + "table_id={}", table_id); return nullptr; } } - LOG_DEBUG(log, "Get Table Info from TiKV : " + table_info_json); + LOG_DEBUG(log, "Get Table Info from TiKV: {}", table_info_json); TiDB::TableInfoPtr table_info = std::make_shared(table_info_json, keyspace_id); return table_info; @@ -319,26 +320,26 @@ std::tuple SchemaGetter::getDatabaseAndTabl if (db_json.empty()) return std::make_tuple(nullptr, nullptr); - LOG_DEBUG(log, "Get DB Info from TiKV : " + db_json); + LOG_DEBUG(log, "Get DB Info from TiKV: {}", db_json); auto db_info = std::make_shared(db_json, keyspace_id); String table_key = getTableKey(table_id); String table_info_json = TxnStructure::hGet(snap, db_key, table_key); if (table_info_json.empty()) { - LOG_WARNING(log, "The table {} is dropped in TiKV, try to get the latest table_info", table_id); + LOG_WARNING(log, "The table is dropped in TiKV, try to get the latest table_info, table_id={}", table_id); table_info_json = TxnStructure::mvccGet(snap, db_key, table_key); if (table_info_json.empty()) { LOG_ERROR( log, - "The table {} is dropped in TiKV, and the latest table_info is still empty, it should by gc", + "The table is dropped in TiKV, and the latest table_info is still empty, it should be GCed, " + "table_id={}", table_id); return std::make_tuple(db_info, nullptr); - ; } } - LOG_DEBUG(log, "Get Table Info from TiKV : " + table_info_json); + LOG_DEBUG(log, "Get Table Info from TiKV: {}", table_info_json); TiDB::TableInfoPtr table_info = std::make_shared(table_info_json, keyspace_id); return std::make_tuple(db_info, table_info); From 0239327919512140c891d5a7264537ca32dc206c Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 9 Nov 2023 21:00:50 +0800 Subject: [PATCH 06/11] Cleanup useless field --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 6 ------ dbms/src/TiDB/Schema/TiDB.cpp | 5 ----- dbms/src/TiDB/Schema/TiDB.h | 2 -- 3 files changed, 13 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index 328769f3894..c9a082e11b4 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -527,12 +527,6 @@ void SchemaBuilder::applyPartitionDiff( } } - if (table_info->partition.new_table_id != DB::InvalidTableID) - { - // TODO: maybe we should create the new_logical_table here? But we don't know - // the database_id of new_logical_table - } - auto alter_lock = storage->lockForAlter(getThreadNameAndID()); storage->alterSchemaChange( alter_lock, diff --git a/dbms/src/TiDB/Schema/TiDB.cpp b/dbms/src/TiDB/Schema/TiDB.cpp index 20dc695a1f2..8b867037627 100644 --- a/dbms/src/TiDB/Schema/TiDB.cpp +++ b/dbms/src/TiDB/Schema/TiDB.cpp @@ -573,11 +573,6 @@ try } num = json->getValue("num"); - - if (json->has("new_table_id")) - { - new_table_id = json->getValue("new_table_id"); - } } catch (const Poco::Exception & e) { diff --git a/dbms/src/TiDB/Schema/TiDB.h b/dbms/src/TiDB/Schema/TiDB.h index 7813bcd459a..26af887824f 100644 --- a/dbms/src/TiDB/Schema/TiDB.h +++ b/dbms/src/TiDB/Schema/TiDB.h @@ -266,8 +266,6 @@ struct PartitionInfo bool enable = false; std::vector definitions; UInt64 num = 0; - - TableID new_table_id = DB::InvalidTableID; }; struct DBInfo From 5f111f8bc6c9a1c7f5bc96b25a210704fa229cdd Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 9 Nov 2023 21:07:20 +0800 Subject: [PATCH 07/11] Add comments --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index c9a082e11b4..ac77e5b10f0 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -462,6 +462,8 @@ void SchemaBuilder::applyPartitionDiff( { const auto & local_table_info = storage->getTableInfo(); // ALTER TABLE t PARTITION BY ... may turn a non-partition table into partition table + // with some partition ids in `partition.adding_definitions`/`partition.definitions` + // and `partition.dropping_definitions`. We need to create those partitions. if (!local_table_info.isLogicalPartitionTable()) { LOG_INFO( From adf8aa184c5d127fdb69420df73f3edf64ac1b50 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 9 Nov 2023 21:11:32 +0800 Subject: [PATCH 08/11] Add comments --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index ac77e5b10f0..a61392209d9 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -1094,12 +1094,12 @@ void SchemaBuilder::applyDropTable(DatabaseID database_id, T if (TableID latest_logical_table_id = table_id_map.findTableIDInPartitionMap(part_def.id); latest_logical_table_id == -1 || latest_logical_table_id != table_info.id) { - // The partition is managed by another logical table now, skip dropping this partition - // when dropping the old logical table + // The partition is managed by another logical table now (caused by `alter table X partition by ...`), + // skip dropping this partition when dropping the old logical table LOG_INFO( log, "The partition is not managed by current logical table, skip, partition_table_id={} " - "new_logical_table_id={} current_table_id={}", + "new_logical_table_id={} current_logical_table_id={}", part_def.id, latest_logical_table_id, table_info.id); From c3d08a7f5ae5ce6bd688b2e145d5fbd62f43c45f Mon Sep 17 00:00:00 2001 From: JaySon Date: Thu, 9 Nov 2023 22:28:23 +0800 Subject: [PATCH 09/11] remove fake workaround --- tests/fullstack-test2/ddl/alter_partition_by.test | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/fullstack-test2/ddl/alter_partition_by.test b/tests/fullstack-test2/ddl/alter_partition_by.test index 51339106290..9c64a7099d3 100644 --- a/tests/fullstack-test2/ddl/alter_partition_by.test +++ b/tests/fullstack-test2/ddl/alter_partition_by.test @@ -176,9 +176,6 @@ mysql> set session tidb_isolation_read_engines='tiflash'; select * from test.t2 mysql> alter table test.t2 partition by hash (a) partitions 3; mysql> analyze table test.t2; -#TODO this is just a workaround, the tidb return before waitting tiflash replica available for the new partitions -SLEEP 10 - mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t2 partition (p0); +----------+ | count(*) | From a8ac79becd0d474c49fe7f13f1a5b2b304ee64a5 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Fri, 10 Nov 2023 10:56:12 +0800 Subject: [PATCH 10/11] Refine logging Signed-off-by: JaySon-Huang --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 30 +++++++++++++------------- dbms/src/TiDB/Schema/SchemaBuilder.h | 2 +- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index a61392209d9..4f92c76a71a 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -81,7 +81,7 @@ void SchemaBuilder::applyCreateTable(DatabaseID database_id, // If table is partition table, we will create the logical table here. // Because we get the table_info, so we can ensure new_db_info will not be nullptr. auto new_db_info = getter.getDatabase(database_id); - applyCreatePhysicalTable(new_db_info, table_info); + applyCreateStorageInstance(new_db_info, table_info); for (const auto & part_def : table_info->partition.definitions) { @@ -399,7 +399,7 @@ void SchemaBuilder::applyPartitionAlter(DatabaseID database_ // If table is partition table, we will create the logical table here. // Because we get the table_info, so we can ensure new_db_info will not be nullptr. auto new_db_info = getter.getDatabase(database_id); - applyCreatePhysicalTable(new_db_info, table_info); + applyCreateStorageInstance(new_db_info, table_info); for (const auto & part_def : table_info->partition.definitions) { @@ -783,7 +783,7 @@ template void SchemaBuilder::applyCreateSchema(const TiDB::DBInfoPtr & db_info) { GET_METRIC(tiflash_schema_internal_ddl_count, type_create_db).Increment(); - LOG_INFO(log, "Creating database {} with database_id={}", name_mapper.debugDatabaseName(*db_info), db_info->id); + LOG_INFO(log, "Create database {} begin, database_id={}", name_mapper.debugDatabaseName(*db_info), db_info->id); auto statement = createDatabaseStmt(context, *db_info, name_mapper); @@ -799,7 +799,7 @@ void SchemaBuilder::applyCreateSchema(const TiDB::DBInfoPtr databases.emplace(db_info->id, db_info); } - LOG_INFO(log, "Created database {} with database_id={}", name_mapper.debugDatabaseName(*db_info), db_info->id); + LOG_INFO(log, "Create database {} end, database_id={}", name_mapper.debugDatabaseName(*db_info), db_info->id); } template @@ -836,7 +836,7 @@ template void SchemaBuilder::applyDropSchema(const String & db_name) { GET_METRIC(tiflash_schema_internal_ddl_count, type_drop_db).Increment(); - LOG_INFO(log, "Tombstoning database {}", db_name); + LOG_INFO(log, "Tombstone database begin, db_name={}", db_name); auto db = context.tryGetDatabase(db_name); if (db == nullptr) { @@ -857,7 +857,7 @@ void SchemaBuilder::applyDropSchema(const String & db_name) auto tombstone = tmt_context.getPDClient()->getTS(); db->alterTombstone(context, tombstone); - LOG_INFO(log, "Tombstoned database {}", db_name); + LOG_INFO(log, "Tombstone database end, db_name={}", db_name); } std::tuple parseColumnsFromTableInfo(const TiDB::TableInfo & table_info) @@ -938,7 +938,7 @@ String createTableStmt( } template -void SchemaBuilder::applyCreatePhysicalTable( +void SchemaBuilder::applyCreateStorageInstance( const TiDB::DBInfoPtr & db_info, const TableInfoPtr & table_info) { @@ -1048,7 +1048,7 @@ void SchemaBuilder::applyDropPhysicalTable(const String & db GET_METRIC(tiflash_schema_internal_ddl_count, type_drop_table).Increment(); LOG_INFO( log, - "Tombstoning table {}.{}, table_id={}", + "Tombstone table {}.{} begin, table_id={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), table_id); @@ -1070,7 +1070,7 @@ void SchemaBuilder::applyDropPhysicalTable(const String & db storage->updateTombstone(alter_lock, commands, db_name, storage->getTableInfo(), name_mapper, context); LOG_INFO( log, - "Tombstoned table {}.{}, table_id={}", + "Tombstone table {}.{} end, table_id={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), table_id); @@ -1120,7 +1120,7 @@ void SchemaBuilder::applyDropTable(DatabaseID database_id, T template void SchemaBuilder::syncAllSchema() { - LOG_INFO(log, "Syncing all schemas."); + LOG_INFO(log, "Sync all schemas begin"); /// Create all databases. std::vector all_schemas = getter.listDBs(); @@ -1181,7 +1181,7 @@ void SchemaBuilder::syncAllSchema() table_id_map.emplaceTableID(table->id, db->id); LOG_DEBUG(log, "register table to table_id_map, database_id={} table_id={}", db->id, table->id); - applyCreatePhysicalTable(db, table); + applyCreateStorageInstance(db, table); if (table->isLogicalPartitionTable()) { for (const auto & part_def : table->partition.definitions) @@ -1238,7 +1238,7 @@ void SchemaBuilder::syncAllSchema() } } - LOG_INFO(log, "Loaded all schemas."); + LOG_INFO(log, "Sync all schemas end"); } template @@ -1295,7 +1295,7 @@ void SchemaBuilder::applyTable( return; } - applyCreatePhysicalTable(db_info, table_info); + applyCreateStorageInstance(db_info, table_info); } else { @@ -1320,7 +1320,7 @@ void SchemaBuilder::applyTable( template void SchemaBuilder::dropAllSchema() { - LOG_INFO(log, "Dropping all schemas."); + LOG_INFO(log, "Drop all schemas begin"); auto & tmt_context = context.getTMTContext(); @@ -1355,7 +1355,7 @@ void SchemaBuilder::dropAllSchema() LOG_DEBUG(log, "DB {} dropped during drop all schemas", db.first); } - LOG_INFO(log, "Dropped all schemas."); + LOG_INFO(log, "Drop all schemas end"); } // product env diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.h b/dbms/src/TiDB/Schema/SchemaBuilder.h index fd4be82d5cf..58150f20fe2 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.h +++ b/dbms/src/TiDB/Schema/SchemaBuilder.h @@ -195,7 +195,7 @@ struct SchemaBuilder void applyCreateSchema(const TiDB::DBInfoPtr & db_info); - void applyCreatePhysicalTable(const TiDB::DBInfoPtr & db_info, const TiDB::TableInfoPtr & table_info); + void applyCreateStorageInstance(const TiDB::DBInfoPtr & db_info, const TiDB::TableInfoPtr & table_info); void applyDropTable(DatabaseID database_id, TableID table_id); From 01a1860ee10b134f60c1cde2b5aabb75abaef397 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Fri, 10 Nov 2023 13:46:22 +0800 Subject: [PATCH 11/11] Replace applyPartitionAlter by applyCreateTable --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 86 +++++++------------------- dbms/src/TiDB/Schema/SchemaBuilder.h | 2 - 2 files changed, 22 insertions(+), 66 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index 4f92c76a71a..bcfd77b100d 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -76,22 +76,26 @@ void SchemaBuilder::applyCreateTable(DatabaseID database_id, table_id_map.emplaceTableID(table_id, database_id); LOG_DEBUG(log, "register table to table_id_map, database_id={} table_id={}", database_id, table_id); - if (table_info->isLogicalPartitionTable()) + // non partition table, done + if (!table_info->isLogicalPartitionTable()) { - // If table is partition table, we will create the logical table here. - // Because we get the table_info, so we can ensure new_db_info will not be nullptr. - auto new_db_info = getter.getDatabase(database_id); - applyCreateStorageInstance(new_db_info, table_info); + return; + } - for (const auto & part_def : table_info->partition.definitions) - { - LOG_DEBUG( - log, - "register table to table_id_map for partition table, logical_table_id={} physical_table_id={}", - table_id, - part_def.id); - table_id_map.emplacePartitionTableID(part_def.id, table_id); - } + // If table is partition table, we will create the logical table here. + // Because we get the table_info, so we can ensure new_db_info will not be nullptr. + auto new_db_info = getter.getDatabase(database_id); + applyCreateStorageInstance(new_db_info, table_info); + + // Register the partition_id -> logical_table_id mapping + for (const auto & part_def : table_info->partition.definitions) + { + LOG_DEBUG( + log, + "register table to table_id_map for partition table, logical_table_id={} physical_table_id={}", + table_id, + part_def.id); + table_id_map.emplacePartitionTableID(part_def.id, table_id); } } @@ -254,9 +258,8 @@ void SchemaBuilder::applyDiff(const SchemaDiff & diff) { // 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 and renew the - // partition info. - applyPartitionAlter(diff.schema_id, diff.table_id); + // the partition id mapping to the new logical table + applyCreateTable(diff.schema_id, diff.table_id); // Drop the old table. if the previous partitions of the old table are // not mapping to the old logical table now, they will not be removed. applyDropTable(diff.schema_id, diff.old_table_id); @@ -378,51 +381,6 @@ void SchemaBuilder::applySetTiFlashReplica(DatabaseID databa } } -template -void SchemaBuilder::applyPartitionAlter(DatabaseID database_id, TableID table_id) -{ - auto table_info = getter.getTableInfo(database_id, table_id); - if (table_info == nullptr) // the database maybe dropped - { - LOG_DEBUG(log, "table is not exist in TiKV, may have been dropped, table_id={}", table_id); - return; - } - - table_id_map.emplaceTableID(table_id, database_id); - LOG_DEBUG(log, "register table to table_id_map, database_id={} table_id={}", database_id, table_id); - - if (!table_info->isLogicalPartitionTable()) - { - return; - } - - // If table is partition table, we will create the logical table here. - // Because we get the table_info, so we can ensure new_db_info will not be nullptr. - auto new_db_info = getter.getDatabase(database_id); - applyCreateStorageInstance(new_db_info, table_info); - - for (const auto & part_def : table_info->partition.definitions) - { - LOG_DEBUG( - log, - "register table to table_id_map for partition table, logical_table_id={} physical_table_id={}", - table_id, - part_def.id); - table_id_map.emplacePartitionTableID(part_def.id, table_id); - } - - auto & tmt_context = context.getTMTContext(); - auto storage = tmt_context.getStorages().get(keyspace_id, table_info->id); - if (storage == nullptr) - { - LOG_ERROR(log, "table is not exist in TiFlash, table_id={}", table_id); - return; - } - - // Try to renew the partition info for the new table - applyPartitionDiff(new_db_info, table_info, storage); -} - template void SchemaBuilder::applyPartitionDiff(DatabaseID database_id, TableID table_id) { @@ -945,7 +903,7 @@ void SchemaBuilder::applyCreateStorageInstance( GET_METRIC(tiflash_schema_internal_ddl_count, type_create_table).Increment(); LOG_INFO( log, - "Creating table {} with database_id={}, table_id={}", + "Create table {} begin, database_id={}, table_id={}", name_mapper.debugCanonicalName(*db_info, *table_info), db_info->id, table_info->id); @@ -1028,7 +986,7 @@ void SchemaBuilder::applyCreateStorageInstance( interpreter.execute(); LOG_INFO( log, - "Created table {}, database_id={} table_id={}", + "Creat table {} end, database_id={} table_id={}", name_mapper.debugCanonicalName(*db_info, *table_info), db_info->id, table_info->id); diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.h b/dbms/src/TiDB/Schema/SchemaBuilder.h index 58150f20fe2..dd9ea341b78 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.h +++ b/dbms/src/TiDB/Schema/SchemaBuilder.h @@ -206,8 +206,6 @@ struct SchemaBuilder /// Parameter schema_name should be mapped. void applyDropPhysicalTable(const String & db_name, TableID table_id); - void applyPartitionAlter(DatabaseID database_id, TableID table_id); - void applyPartitionDiff(DatabaseID database_id, TableID table_id); void applyPartitionDiff( const TiDB::DBInfoPtr & db_info,