From 8ed983598006ac5221574e441972488bfd458133 Mon Sep 17 00:00:00 2001 From: JaySon Date: Thu, 1 Aug 2024 12:08:50 +0800 Subject: [PATCH] ddl: Fix rename come after database has been dropped (release-7.5) (#9275) close pingcap/tiflash#9266 --- dbms/src/Debug/MockTiDB.cpp | 9 +-- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 63 +++++++++++-------- dbms/src/TiDB/Schema/SchemaBuilder.h | 1 + .../ddl/rename_table_across_databases.test | 22 +++++++ 4 files changed, 64 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..588c7282681 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 (likely(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 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 + 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/tests/fullstack-test2/ddl/rename_table_across_databases.test b/tests/fullstack-test2/ddl/rename_table_across_databases.test index e9b3274a74d..0978fd42c16 100644 --- a/tests/fullstack-test2/ddl/rename_table_across_databases.test +++ b/tests/fullstack-test2/ddl/rename_table_across_databases.test @@ -126,3 +126,25 @@ 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) +>> DBGInvoke __refresh_schemas()