ddl: Fix corner case when multiple region creates one table simultane…

…ously (#8229) (#8238)

close #8217

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)

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;
 

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);
 }
 

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;
 

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(

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;
 

dbms/src/Databases/IDatabase.h

@@ -88,12 +88,7 @@ class IDatabase : public std::enable_shared_from_this<IDatabase>
     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;

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.

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;