diff --git a/dbms/src/Databases/DatabaseMemory.cpp b/dbms/src/Databases/DatabaseMemory.cpp index 46c8e0dc035..5e1b442636f 100644 --- a/dbms/src/Databases/DatabaseMemory.cpp +++ b/dbms/src/Databases/DatabaseMemory.cpp @@ -37,13 +37,9 @@ void DatabaseMemory::loadTables( /// Nothing to load. } -void DatabaseMemory::createTable( - const Context & /*context*/, - const String & table_name, - const StoragePtr & table, - const ASTPtr & /*query*/) +void DatabaseMemory::createTable(const Context & /*context*/, const String & /*table_name*/, const ASTPtr & /*query*/) { - attachTable(table_name, table); + /// Nothing to do. } void DatabaseMemory::removeTable(const Context & /*context*/, const String & table_name) diff --git a/dbms/src/Databases/DatabaseMemory.h b/dbms/src/Databases/DatabaseMemory.h index 0046113165f..f89e987c2b5 100644 --- a/dbms/src/Databases/DatabaseMemory.h +++ b/dbms/src/Databases/DatabaseMemory.h @@ -40,8 +40,7 @@ class DatabaseMemory : public DatabaseWithOwnTablesBase void loadTables(Context & context, ThreadPool * thread_pool, bool has_force_restore_data_flag) override; - void createTable(const Context & context, const String & table_name, const StoragePtr & table, const ASTPtr & query) - override; + void createTable(const Context & context, const String & table_name, const ASTPtr & query) override; void removeTable(const Context & context, const String & table_name) override; diff --git a/dbms/src/Databases/DatabaseOrdinary.cpp b/dbms/src/Databases/DatabaseOrdinary.cpp index ba29e25855f..5d7c4030595 100644 --- a/dbms/src/Databases/DatabaseOrdinary.cpp +++ b/dbms/src/Databases/DatabaseOrdinary.cpp @@ -181,11 +181,7 @@ void DatabaseOrdinary::loadTables(Context & context, ThreadPool * thread_pool, b DatabaseLoading::cleanupTables(*this, name, tables_failed_to_startup, log); } -void DatabaseOrdinary::createTable( - const Context & context, - const String & table_name, - const StoragePtr & table, - const ASTPtr & query) +void DatabaseOrdinary::createTable(const Context & context, const String & table_name, const ASTPtr & query) { const auto & settings = context.getSettingsRef(); @@ -234,15 +230,6 @@ void DatabaseOrdinary::createTable( try { - /// Add a table to the map of known tables. - { - std::lock_guard lock(mutex); - if (!tables.emplace(table_name, table).second) - throw Exception( - fmt::format("Table {}.{} already exists.", name, table_name), - ErrorCodes::TABLE_ALREADY_EXISTS); - } - context.getFileProvider()->renameFile( table_metadata_tmp_path, EncryptionPath(table_metadata_tmp_path, ""), @@ -331,7 +318,8 @@ void DatabaseOrdinary::renameTable( /// NOTE Non-atomic. // Create new metadata and remove old metadata. - to_database_concrete->createTable(context, to_table_name, table, ast); + to_database_concrete->createTable(context, to_table_name, ast); + to_database_concrete->attachTable(to_table_name, table); removeTable(context, table_name); } diff --git a/dbms/src/Databases/DatabaseOrdinary.h b/dbms/src/Databases/DatabaseOrdinary.h index 26373a229d9..597ca558bf2 100644 --- a/dbms/src/Databases/DatabaseOrdinary.h +++ b/dbms/src/Databases/DatabaseOrdinary.h @@ -32,8 +32,7 @@ class DatabaseOrdinary : public DatabaseWithOwnTablesBase void loadTables(Context & context, ThreadPool * thread_pool, bool has_force_restore_data_flag) override; - void createTable(const Context & context, const String & table_name, const StoragePtr & table, const ASTPtr & query) - override; + void createTable(const Context & context, const String & table_name, const ASTPtr & query) override; void removeTable(const Context & context, const String & table_name) override; diff --git a/dbms/src/Databases/DatabaseTiFlash.cpp b/dbms/src/Databases/DatabaseTiFlash.cpp index 279d0735bd7..f7b300e1efc 100644 --- a/dbms/src/Databases/DatabaseTiFlash.cpp +++ b/dbms/src/Databases/DatabaseTiFlash.cpp @@ -209,11 +209,7 @@ void DatabaseTiFlash::loadTables(Context & context, ThreadPool * thread_pool, bo } -void DatabaseTiFlash::createTable( - const Context & context, - const String & table_name, - const StoragePtr & table, - const ASTPtr & query) +void DatabaseTiFlash::createTable(const Context & context, const String & table_name, const ASTPtr & query) { const auto & settings = context.getSettingsRef(); @@ -259,15 +255,6 @@ void DatabaseTiFlash::createTable( try { - /// Add a table to the map of known tables. - { - std::lock_guard lock(mutex); - if (!tables.emplace(table_name, table).second) - throw Exception( - "Table " + name + "." + table_name + " already exists.", - ErrorCodes::TABLE_ALREADY_EXISTS); - } - /// If it was ATTACH query and file with table metadata already exist /// (so, ATTACH is done after DETACH), then rename atomically replaces old file with new one. context.getFileProvider()->renameFile( diff --git a/dbms/src/Databases/DatabaseTiFlash.h b/dbms/src/Databases/DatabaseTiFlash.h index d0a5082e21c..178e7e00c39 100644 --- a/dbms/src/Databases/DatabaseTiFlash.h +++ b/dbms/src/Databases/DatabaseTiFlash.h @@ -45,8 +45,7 @@ class DatabaseTiFlash : public DatabaseWithOwnTablesBase void loadTables(Context & context, ThreadPool * thread_pool, bool has_force_restore_data_flag) override; - void createTable(const Context & context, const String & table_name, const StoragePtr & table, const ASTPtr & query) - override; + void createTable(const Context & context, const String & table_name, const ASTPtr & query) override; void removeTable(const Context & context, const String & table_name) override; diff --git a/dbms/src/Databases/IDatabase.h b/dbms/src/Databases/IDatabase.h index db1d24f6bca..15fdce40c40 100644 --- a/dbms/src/Databases/IDatabase.h +++ b/dbms/src/Databases/IDatabase.h @@ -88,12 +88,7 @@ class IDatabase : public std::enable_shared_from_this virtual bool empty(const Context & context) const = 0; /// Add the table to the database. Record its presence in the metadata. - virtual void createTable( - const Context & context, - const String & name, - const StoragePtr & table, - const ASTPtr & query) - = 0; + virtual void createTable(const Context & context, const String & name, const ASTPtr & query) = 0; /// Delete the table from the database and return it. Delete the metadata. virtual void removeTable(const Context & context, const String & name) = 0; diff --git a/dbms/src/Interpreters/InterpreterCreateQuery.cpp b/dbms/src/Interpreters/InterpreterCreateQuery.cpp index 4cb4d112c71..d20c3466e78 100644 --- a/dbms/src/Interpreters/InterpreterCreateQuery.cpp +++ b/dbms/src/Interpreters/InterpreterCreateQuery.cpp @@ -624,13 +624,24 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) create.attach, false); - // start up before adding to `database`, or the storage can not be retrieved from `ManagedStorages::get` - res->startup(); + // When creating tables, we should strictly follow the following steps: + // 1. Create a .sql file + // 2. Register the instance to ManagedStorages + // 3. Register the instance to IDatabase + // Once the instance is registered in `ManagedStorages`, we will try to apply DDL alter changes to its .sql files + // If we do step 2 before step 1, we may run into "can't find .sql file" error when applying DDL jobs. + // Besides, we make step 3 the final one, to ensure once we pass the check of context.isTableExist(database_name, table_name)`, the table must be created completely. if (create.is_temporary) context.getSessionContext().addExternalTable(table_name, res, query_ptr); else - database->createTable(context, table_name, res, query_ptr); + database->createTable(context, table_name, query_ptr); + + // register the storage instance into `ManagedStorages` + res->startup(); + + if (!create.is_temporary) + database->attachTable(table_name, res); } /// If the query is a CREATE SELECT, insert the data into the table. diff --git a/tests/fullstack-test2/ddl/alter_create_table_from_multi_regions.test b/tests/fullstack-test2/ddl/alter_create_table_from_multi_regions.test new file mode 100644 index 00000000000..80d879b3127 --- /dev/null +++ b/tests/fullstack-test2/ddl/alter_create_table_from_multi_regions.test @@ -0,0 +1,93 @@ +# Copyright 2023 PingCAP, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# mock a table with multiple regions, and reach tiflash together, +# to make multiple regions create the same table simultaneously + +mysql> drop database if exists d1; +mysql> create database d1; +mysql> create table d1.t1 (id int not null primary key); +mysql> split table d1.t1 between (0) and (50000) regions 50; ++--------------------+------------------------+ +| TOTAL_SPLIT_REGION | SCATTER_FINISH_RATIO | +| 49 | 1 | ++--------------------+------------------------+ + +mysql> insert into d1.t1 values (1), (1001), (2001), (3001), (4001), (5001), (6001), (7001), (8001),(9001); +mysql> insert into d1.t1 values (10001), (11001), (12001), (13001), (14001), (15001), (16001), (17001), (18001),(19001); +mysql> insert into d1.t1 values (20001), (21001), (22001), (23001), (24001), (25001), (26001), (27001), (28001),(29001); +mysql> insert into d1.t1 values (30001), (31001), (32001), (33001), (34001), (35001), (36001), (37001), (38001),(39001); +mysql> insert into d1.t1 values (40001), (41001), (42001), (43001), (44001), (45001), (46001), (47001), (48001),(49001); + +mysql> alter table d1.t1 set tiflash replica 1; + +func> wait_table d1 t1 +mysql> set session tidb_isolation_read_engines='tiflash';select * from d1.t1; ++------+ +| id | +| 1 | +| 1001 | +| 2001 | +| 3001 | +| 4001 | +| 5001 | +| 6001 | +| 7001 | +| 8001 | +| 9001 | +| 10001| +| 11001| +| 12001| +| 13001| +| 14001| +| 15001| +| 16001| +| 17001| +| 18001| +| 19001| +| 20001| +| 21001| +| 22001| +| 23001| +| 24001| +| 25001| +| 26001| +| 27001| +| 28001| +| 29001| +| 30001| +| 31001| +| 32001| +| 33001| +| 34001| +| 35001| +| 36001| +| 37001| +| 38001| +| 39001| +| 40001| +| 41001| +| 42001| +| 43001| +| 44001| +| 45001| +| 46001| +| 47001| +| 48001| +| 49001| ++------+ + + +mysql> drop table d1.t1; +mysql> drop database d1;