Merge pull request #23189 from ClickHouse/minor_fixes_in_attach_query

Minor fixes in ATTACH query

src/Databases/DatabaseAtomic.cpp

@@ -365,8 +365,8 @@ void DatabaseAtomic::assertDetachedTableNotInUse(const UUID & uuid)
     /// 4. INSERT INTO table ...; (both Storage instances writes data without any synchronization)
     /// To avoid it, we remember UUIDs of detached tables and does not allow ATTACH table with such UUID until detached instance still in use.
     if (detached_tables.count(uuid))
-        throw Exception("Cannot attach table with UUID " + toString(uuid) +
-              ", because it was detached but still used by some query. Retry later.", ErrorCodes::TABLE_ALREADY_EXISTS);
+        throw Exception(ErrorCodes::TABLE_ALREADY_EXISTS, "Cannot attach table with UUID {}, "
+                        "because it was detached but still used by some query. Retry later.", toString(uuid));
 }
 
 void DatabaseAtomic::setDetachedTableNotInUseForce(const UUID & uuid)
@@ -573,12 +573,6 @@ void DatabaseAtomic::renameDictionaryInMemoryUnlocked(const StorageID & old_name
 }
 void DatabaseAtomic::waitDetachedTableNotInUse(const UUID & uuid)
 {
-    {
-        std::lock_guard lock{mutex};
-        if (detached_tables.count(uuid) == 0)
-            return;
-    }
-
     /// Table is in use while its shared_ptr counter is greater than 1.
     /// We cannot trigger condvar on shared_ptr destruction, so it's busy wait.
     while (true)
@@ -594,5 +588,13 @@ void DatabaseAtomic::waitDetachedTableNotInUse(const UUID & uuid)
     }
 }
 
+void DatabaseAtomic::checkDetachedTableNotInUse(const UUID & uuid)
+{
+    DetachedTables not_in_use;
+    std::lock_guard lock{mutex};
+    not_in_use = cleanupDetachedTables();
+    assertDetachedTableNotInUse(uuid);
+}
+
 }
 

src/Databases/DatabaseAtomic.h

@@ -58,6 +58,7 @@ public:
     void tryRemoveSymlink(const String & table_name);
 
     void waitDetachedTableNotInUse(const UUID & uuid) override;
+    void checkDetachedTableNotInUse(const UUID & uuid) override;
     void setDetachedTableNotInUseForce(const UUID & uuid);
 
 protected:

src/Databases/DatabaseWithDictionaries.cpp

@@ -169,11 +169,22 @@ void DatabaseWithDictionaries::createDictionary(ContextPtr local_context, const
     }
 
     bool succeeded = false;
+    bool uuid_locked = false;
     SCOPE_EXIT({
         if (!succeeded)
+        {
+            if (uuid_locked)
+                DatabaseCatalog::instance().removeUUIDMappingFinally(dict_id.uuid);
             Poco::File(dictionary_metadata_tmp_path).remove();
+        }
     });
 
+    if (dict_id.uuid != UUIDHelpers::Nil)
+    {
+        DatabaseCatalog::instance().addUUIDMapping(dict_id.uuid);
+        uuid_locked = true;
+    }
+
     /// Add a temporary repository containing the dictionary.
     /// We need this temp repository to try loading the dictionary before actually attaching it to the database.
     auto temp_repository = external_loader.addConfigRepository(std::make_unique<ExternalLoaderTempConfigRepository>(

src/Databases/IDatabase.h

@@ -345,7 +345,8 @@ public:
 
     virtual void assertCanBeDetached(bool /*cleanup*/) {}
 
-    virtual void waitDetachedTableNotInUse(const UUID & /*uuid*/) { assert(false); }
+    virtual void waitDetachedTableNotInUse(const UUID & /*uuid*/) { }
+    virtual void checkDetachedTableNotInUse(const UUID & /*uuid*/) { }
 
     /// Ask all tables to complete the background threads they are using and delete all table objects.
     virtual void shutdown() = 0;

src/Interpreters/InterpreterCreateQuery.cpp

@@ -996,6 +996,19 @@ bool InterpreterCreateQuery::doCreateTable(ASTCreateQuery & create,
         create.attach_from_path = std::nullopt;
     }
 
+    if (create.attach)
+    {
+        /// If table was detached it's not possible to attach it back while some threads are using
+        /// old instance of the storage. For example, AsynchronousMetrics may cause ATTACH to fail,
+        /// so we allow waiting here. If database_atomic_wait_for_drop_and_detach_synchronously is disabled
+        /// and old storage instance still exists it will throw exception.
+        bool throw_if_table_in_use = getContext()->getSettingsRef().database_atomic_wait_for_drop_and_detach_synchronously;
+        if (throw_if_table_in_use)
+            database->checkDetachedTableNotInUse(create.uuid);
+        else
+            database->waitDetachedTableNotInUse(create.uuid);
+    }
+
     StoragePtr res;
     /// NOTE: CREATE query may be rewritten by Storage creator or table function
     if (create.as_table_function)