review suggestions

src/Databases/DatabaseAtomic.cpp

@@ -261,22 +261,29 @@ void DatabaseAtomic::commitCreateTable(const ASTCreateQuery & query, const Stora
 {
     DetachedTables not_in_use;
     auto table_data_path = getTableDataPath(query);
+    bool locked_uuid = false;
     try
     {
         std::unique_lock lock{mutex};
         if (query.database != database_name)
             throw Exception(ErrorCodes::UNKNOWN_DATABASE, "Database was renamed to `{}`, cannot create table in `{}`",
                             database_name, query.database);
+        /// Do some checks before renaming file from .tmp to .sql
         not_in_use = cleanupDetachedTables();
         assertDetachedTableNotInUse(query.uuid);
-        DatabaseCatalog::instance().addUUIDMapping(query.uuid, {}, {});
-        renameNoReplace(table_metadata_tmp_path, table_metadata_path);
+        /// We will get en exception if some table with the same UUID exists (even if it's detached table or table from another database)
+        DatabaseCatalog::instance().addUUIDMapping(query.uuid);
+        locked_uuid = true;
+        /// It throws if `table_metadata_path` already exists (it's possible if table was detached)
+        renameNoReplace(table_metadata_tmp_path, table_metadata_path);  /// Commit point (a sort of)
         attachTableUnlocked(query.table, table, lock);   /// Should never throw
         table_name_to_path.emplace(query.table, table_data_path);
     }
     catch (...)
     {
         Poco::File(table_metadata_tmp_path).remove();
+        if (locked_uuid)
+            DatabaseCatalog::instance().removeUUIDMappingFinally(query.uuid);
         throw;
     }
     tryCreateSymlink(query.table, table_data_path);

src/Interpreters/DatabaseCatalog.cpp

@@ -164,7 +164,12 @@ void DatabaseCatalog::shutdownImpl()
     std::lock_guard lock(databases_mutex);
     assert(std::find_if(uuid_map.begin(), uuid_map.end(), [](const auto & elem)
     {
-        const auto & not_empty_mapping = [] (const auto & mapping) { return mapping.second.second; };
+        /// Ensure that all UUID mappings are emtpy (i.e. all mappings contain nullptr instead of a pointer to storage)
+        const auto & not_empty_mapping = [] (const auto & mapping)
+        {
+            auto & table = mapping.second.second;
+            return table;
+        };
         auto it = std::find_if(elem.map.begin(), elem.map.end(), not_empty_mapping);
         return it != elem.map.end();
     }) == uuid_map.end());
@@ -423,6 +428,11 @@ DatabasePtr DatabaseCatalog::getSystemDatabase() const
     return getDatabase(SYSTEM_DATABASE);
 }
 
+void DatabaseCatalog::addUUIDMapping(const UUID & uuid)
+{
+    addUUIDMapping(uuid, nullptr, nullptr);
+}
+
 void DatabaseCatalog::addUUIDMapping(const UUID & uuid, const DatabasePtr & database, const StoragePtr & table)
 {
     assert(uuid != UUIDHelpers::Nil && getFirstLevelIdx(uuid) < uuid_map.size());
@@ -744,7 +754,7 @@ void DatabaseCatalog::enqueueDroppedTableCleanup(StorageID table_id, StoragePtr
             LOG_WARNING(log, "Cannot parse metadata of partially dropped table {} from {}. Will remove metadata file and data directory. Garbage may be left in /store directory and ZooKeeper.", table_id.getNameForLogs(), dropped_metadata_path);
         }
 
-        addUUIDMapping(table_id.uuid, {}, {});
+        addUUIDMapping(table_id.uuid);
         drop_time = Poco::File(dropped_metadata_path).getLastModified().epochTime();
     }
 
@@ -786,7 +796,7 @@ void DatabaseCatalog::dropTableDataTask()
         }
         else
         {
-            LOG_TRACE(log, "No tables to drop. Queue size: {}", tables_marked_dropped.size());
+            LOG_TRACE(log, "Not found any suitable tables to drop, still have {} tables in drop queue", tables_marked_dropped.size());
         }
         need_reschedule = !tables_marked_dropped.empty();
     }

src/Interpreters/DatabaseCatalog.h

@@ -167,13 +167,19 @@ public:
     /// If table has UUID, addUUIDMapping(...) must be called when table attached to some database
     /// removeUUIDMapping(...) must be called when it detached,
     /// and removeUUIDMappingFinally(...) must be called when table is dropped and its data removed from disk.
-    /// To "lock" some UUID and prevent collision, addUUIDMapping(...) may be called with nullptr arguments.
     /// Such tables can be accessed by persistent UUID instead of database and table name.
     void addUUIDMapping(const UUID & uuid, const DatabasePtr & database, const StoragePtr & table);
     void removeUUIDMapping(const UUID & uuid);
     void removeUUIDMappingFinally(const UUID & uuid);
     /// For moving table between databases
     void updateUUIDMapping(const UUID & uuid, DatabasePtr database, StoragePtr table);
+    /// This method adds empty mapping (with database and storage equal to nullptr).
+    /// It's required to "lock" some UUIDs and protect us from collision.
+    /// Collisions of random 122-bit integers are very unlikely to happen,
+    /// but we allow to explicitly specify UUID in CREATE query (in particular for testing).
+    /// If some UUID was already added and we are trying to add it again,
+    /// this method will throw an exception.
+    void addUUIDMapping(const UUID & uuid);
 
     static String getPathForUUID(const UUID & uuid);