From 5ecc8c3e0002077328d3fec109ec7029bd176eaf Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 31 Jul 2024 19:39:13 +0800 Subject: [PATCH 1/5] Add test case --- .../ddl/rename_table_across_databases.test | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/fullstack-test2/ddl/rename_table_across_databases.test b/tests/fullstack-test2/ddl/rename_table_across_databases.test index e9b3274a74d..694fa339ee1 100644 --- a/tests/fullstack-test2/ddl/rename_table_across_databases.test +++ b/tests/fullstack-test2/ddl/rename_table_across_databases.test @@ -126,3 +126,24 @@ mysql> set session tidb_isolation_read_engines='tiflash'; select * from test_new +----+----------+------+ mysql> drop table if exists test_new.part4 + +# (case 4) rename partitioned table across database +# (required) stop regular schema sync +=> DBGInvoke __enable_schema_sync_service('false') +mysql> drop database if exists test_new; +mysql> drop table if exists test.part5; +mysql> CREATE TABLE test.part5 (id INT NOT NULL,store_id INT NOT NULL)PARTITION BY RANGE (store_id) (PARTITION p0 VALUES LESS THAN (6),PARTITION p1 VALUES LESS THAN (11),PARTITION p2 VALUES LESS THAN (16),PARTITION p3 VALUES LESS THAN (21)); +# (1,1),(2,2),(3,3) => p0; p1 is empty;(11,11) => p2;(16,16) => p3 +mysql> alter table test.part5 set tiflash replica 1; +func> wait_table test part5 + +>> DBGInvoke __enable_fail_point(pause_before_apply_raft_cmd) +mysql> insert into test.part5(id, store_id) values(1,1),(2,2),(3,3),(11,11),(16,16); + +# create target db, rename table to target db, then drop target db +mysql> create database if not exists test_new; +mysql> rename table test.part5 to test_new.part5; +mysql> alter table test_new.part5 add column c1 int; +mysql> drop database if exists test_new; +# raft command comes after target db dropped, no crashes +>> DBGInvoke __disable_fail_point(pause_before_apply_raft_cmd) From 994cc6f9ce72734714071a3cbf34037a116cfc6b Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 31 Jul 2024 19:45:01 +0800 Subject: [PATCH 2/5] Fix raft log and rename comes after database is dropped --- dbms/src/Debug/MockTiDB.cpp | 9 +-- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 63 +++++++++++-------- dbms/src/TiDB/Schema/SchemaBuilder.h | 1 + .../TiDB/Schema/tests/gtest_schema_sync.cpp | 46 ++++++++++++++ 4 files changed, 88 insertions(+), 31 deletions(-) diff --git a/dbms/src/Debug/MockTiDB.cpp b/dbms/src/Debug/MockTiDB.cpp index c94f0f00b43..8fb7ee36619 100644 --- a/dbms/src/Debug/MockTiDB.cpp +++ b/dbms/src/Debug/MockTiDB.cpp @@ -875,17 +875,18 @@ TiDB::TableInfoPtr MockTiDB::getTableInfoByID(TableID table_id) TiDB::DBInfoPtr MockTiDB::getDBInfoByID(DatabaseID db_id) { - TiDB::DBInfoPtr db_ptr = std::make_shared(TiDB::DBInfo()); - db_ptr->id = db_id; for (const auto & database : databases) { if (database.second == db_id) { + TiDB::DBInfoPtr db_ptr = std::make_shared(TiDB::DBInfo()); + db_ptr->id = db_id; db_ptr->name = database.first; - break; + return db_ptr; } } - return db_ptr; + // If the database has been dropped in TiKV, TiFlash get a nullptr + return nullptr; } std::pair MockTiDB::getDBIDByName(const String & database_name) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index 35f71624110..9742f471e21 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -25,16 +25,14 @@ #include #include #include -#include #include -#include #include #include -#include #include #include #include #include +#include #include #include #include @@ -727,6 +725,11 @@ void SchemaBuilder::applyRenamePhysicalTable( return; } + // There could be a chance that the target database has been dropped in TiKV before + // TiFlash accepts the "create database" schema diff. We need to ensure the local + // database exist before executing renaming. + const auto action = fmt::format("applyRenamePhysicalTable-table_id={}", new_table_info.id); + ensureLocalDatabaseExist(new_database_id, new_mapped_db_name, action); const auto old_mapped_tbl_name = storage->getTableName(); GET_METRIC(tiflash_schema_internal_ddl_count, type_rename_table).Increment(); LOG_INFO( @@ -1171,28 +1174,7 @@ void SchemaBuilder::applyCreateStorageInstance( // in TiDB, then TiFlash may not create the IDatabase instance. Make sure we can access // to the IDatabase when creating IStorage. const auto database_mapped_name = name_mapper.mapDatabaseName(database_id, keyspace_id); - if (!context.isDatabaseExist(database_mapped_name)) - { - LOG_WARNING( - log, - "database instance is not exist (applyCreateStorageInstance), may has been dropped, create a database " - "with " - "fake DatabaseInfo for it, database_id={} database_name={} action={}", - database_id, - database_mapped_name, - action); - // The database is dropped in TiKV and we can not fetch it. Generate a fake - // DatabaseInfo for it. It is OK because the DatabaseInfo will be updated - // when the database is `FLASHBACK`. - TiDB::DBInfoPtr database_info = std::make_shared(); - database_info->id = database_id; - database_info->keyspace_id = keyspace_id; - database_info->name = database_mapped_name; // use the mapped name because we done known the actual name - database_info->charset = "utf8mb4"; // default value - database_info->collate = "utf8mb4_bin"; // default value - database_info->state = TiDB::StateNone; // special state - applyCreateDatabaseByInfo(database_info); - } + ensureLocalDatabaseExist(database_id, database_mapped_name, action); ParserCreateQuery parser; ASTPtr ast = parseQuery(parser, stmt.data(), stmt.data() + stmt.size(), "from syncSchema " + table_info->name, 0); @@ -1208,13 +1190,41 @@ void SchemaBuilder::applyCreateStorageInstance( interpreter.execute(); LOG_INFO( log, - "Creat table {} end, database_id={} table_id={} action={}", + "Create table {} end, database_id={} table_id={} action={}", name_mapper.debugCanonicalName(*table_info, database_id, keyspace_id), database_id, table_info->id, action); } +template +void SchemaBuilder::ensureLocalDatabaseExist( + DatabaseID database_id, + const String & database_mapped_name, + std::string_view action) +{ + if (context.isDatabaseExist(database_mapped_name)) + return; + + LOG_WARNING( + log, + "database instance is not exist, may has been dropped, create a database " + "with fake DatabaseInfo for it, database_id={} database_name={} action={}", + database_id, + database_mapped_name, + action); + // The database is dropped in TiKV and we can not fetch it. Generate a fake + // DatabaseInfo for it. It is OK because the DatabaseInfo will be updated + // when the database is `FLASHBACK`. + TiDB::DBInfoPtr database_info = std::make_shared(); + database_info->id = database_id; + database_info->keyspace_id = keyspace_id; + database_info->name = database_mapped_name; // use the mapped name because we done known the actual name + database_info->charset = "utf8mb4"; // default value + database_info->collate = "utf8mb4_bin"; // default value + database_info->state = TiDB::StateNone; // special state + applyCreateDatabaseByInfo(database_info); +} template void SchemaBuilder::applyDropPhysicalTable( @@ -1775,5 +1785,4 @@ template struct SchemaBuilder; template struct SchemaBuilder; // unit test template struct SchemaBuilder; -// end namespace } // namespace DB diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.h b/dbms/src/TiDB/Schema/SchemaBuilder.h index ceb58401bd5..1342ba3f5fd 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.h +++ b/dbms/src/TiDB/Schema/SchemaBuilder.h @@ -72,6 +72,7 @@ struct SchemaBuilder bool applyCreateDatabase(DatabaseID database_id); void applyCreateDatabaseByInfo(const TiDB::DBInfoPtr & db_info); + void ensureLocalDatabaseExist(DatabaseID database_id, const String & database_mapped_name, std::string_view action); void applyRecoverDatabase(DatabaseID database_id); diff --git a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp index 14b1e6ad2f7..5f27a630641 100644 --- a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp +++ b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp @@ -560,6 +560,52 @@ try } CATCH +TEST_F(SchemaSyncTest, RenamePartitionTableAcrossDatabaseNoTargetDB) +try +{ + auto pd_client = global_ctx.getTMTContext().getPDClient(); + + const String db_name = "mock_db"; + const String new_db_name = "mock_new_db"; + const String tbl_name = "mock_part_tbl"; + + auto cols = ColumnsDescription({ + {"col1", typeFromString("String")}, + {"col2", typeFromString("Int64")}, + }); + + MockTiDB::instance().newDataBase(db_name); + + auto [logical_table_id, physical_table_ids] = MockTiDB::instance().newPartitionTable( // + db_name, + tbl_name, + cols, + pd_client->getTS(), + "", + "dt", + {"red", "blue", "yellow"}); + + ASSERT_EQ(physical_table_ids.size(), 3); + + // create "mock_db"."mock_part_tbl" in tiflash + refreshSchema(); + + // Mock that create target database, rename to the target database, drop target database + MockTiDB::instance().newDataBase(new_db_name); + const String new_tbl_name = "mock_part_tbl_renamed"; + MockTiDB::instance().renameTableTo(db_name, tbl_name, new_db_name, new_tbl_name); + MockTiDB::instance().dropDB(global_ctx, new_db_name, false); + + // After target database has been dropped, then tiflash receive the schema diff + refreshSchema(); + refreshTableSchema(logical_table_id); + refreshTableSchema(physical_table_ids[0]); + // refreshTableSchema(physical_table_ids[1]); // mock that physical_table[1] have no data neither read + refreshTableSchema(physical_table_ids[2]); +} +CATCH + + TEST_F(SchemaSyncTest, RenamePartitionTableAcrossDatabaseWithRestart) try { From 55c53514a13f4943bd1d917728fc922ea0511210 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 31 Jul 2024 19:45:33 +0800 Subject: [PATCH 3/5] Remove useless test --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 2 +- .../TiDB/Schema/tests/gtest_schema_sync.cpp | 46 ------------------- 2 files changed, 1 insertion(+), 47 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index 9742f471e21..ed2576911c7 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -1203,7 +1203,7 @@ void SchemaBuilder::ensureLocalDatabaseExist( const String & database_mapped_name, std::string_view action) { - if (context.isDatabaseExist(database_mapped_name)) + if (likely(context.isDatabaseExist(database_mapped_name))) return; LOG_WARNING( diff --git a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp index 5f27a630641..14b1e6ad2f7 100644 --- a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp +++ b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp @@ -560,52 +560,6 @@ try } CATCH -TEST_F(SchemaSyncTest, RenamePartitionTableAcrossDatabaseNoTargetDB) -try -{ - auto pd_client = global_ctx.getTMTContext().getPDClient(); - - const String db_name = "mock_db"; - const String new_db_name = "mock_new_db"; - const String tbl_name = "mock_part_tbl"; - - auto cols = ColumnsDescription({ - {"col1", typeFromString("String")}, - {"col2", typeFromString("Int64")}, - }); - - MockTiDB::instance().newDataBase(db_name); - - auto [logical_table_id, physical_table_ids] = MockTiDB::instance().newPartitionTable( // - db_name, - tbl_name, - cols, - pd_client->getTS(), - "", - "dt", - {"red", "blue", "yellow"}); - - ASSERT_EQ(physical_table_ids.size(), 3); - - // create "mock_db"."mock_part_tbl" in tiflash - refreshSchema(); - - // Mock that create target database, rename to the target database, drop target database - MockTiDB::instance().newDataBase(new_db_name); - const String new_tbl_name = "mock_part_tbl_renamed"; - MockTiDB::instance().renameTableTo(db_name, tbl_name, new_db_name, new_tbl_name); - MockTiDB::instance().dropDB(global_ctx, new_db_name, false); - - // After target database has been dropped, then tiflash receive the schema diff - refreshSchema(); - refreshTableSchema(logical_table_id); - refreshTableSchema(physical_table_ids[0]); - // refreshTableSchema(physical_table_ids[1]); // mock that physical_table[1] have no data neither read - refreshTableSchema(physical_table_ids[2]); -} -CATCH - - TEST_F(SchemaSyncTest, RenamePartitionTableAcrossDatabaseWithRestart) try { From 2bac1aca3dba3fa7071900705caeb730bc86d1a8 Mon Sep 17 00:00:00 2001 From: JaySon Date: Thu, 1 Aug 2024 09:27:50 +0800 Subject: [PATCH 4/5] Apply suggestions from code review --- tests/fullstack-test2/ddl/rename_table_across_databases.test | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/fullstack-test2/ddl/rename_table_across_databases.test b/tests/fullstack-test2/ddl/rename_table_across_databases.test index 694fa339ee1..0978fd42c16 100644 --- a/tests/fullstack-test2/ddl/rename_table_across_databases.test +++ b/tests/fullstack-test2/ddl/rename_table_across_databases.test @@ -147,3 +147,4 @@ mysql> alter table test_new.part5 add column c1 int; mysql> drop database if exists test_new; # raft command comes after target db dropped, no crashes >> DBGInvoke __disable_fail_point(pause_before_apply_raft_cmd) +>> DBGInvoke __refresh_schemas() From 498d840d9042ac89e7311fb74c7b4074d8a601ad Mon Sep 17 00:00:00 2001 From: JaySon Date: Thu, 1 Aug 2024 10:03:32 +0800 Subject: [PATCH 5/5] Apply suggestions from code review --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index ed2576911c7..588c7282681 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -1219,7 +1219,7 @@ void SchemaBuilder::ensureLocalDatabaseExist( TiDB::DBInfoPtr database_info = std::make_shared(); database_info->id = database_id; database_info->keyspace_id = keyspace_id; - database_info->name = database_mapped_name; // use the mapped name because we done known the actual name + database_info->name = database_mapped_name; // use the mapped name because we don't known the actual name database_info->charset = "utf8mb4"; // default value database_info->collate = "utf8mb4_bin"; // default value database_info->state = TiDB::StateNone; // special state