fix symlinks, add short syntax of attach database

programs/copier/ClusterCopierApp.cpp

@@ -114,7 +114,7 @@ void ClusterCopierApp::mainImpl()
     registerDisks();
 
     static const std::string default_database = "_local";
-    DatabaseCatalog::instance().attachDatabase(default_database, std::make_shared<DatabaseMemory>(default_database, *context));
+    DatabaseCatalog::instance().attachDatabase(default_database, UUIDHelpers::Nil, std::make_shared<DatabaseMemory>(default_database, *context));
     context->setCurrentDatabase(default_database);
 
     /// Initialize query scope just in case.

programs/local/LocalServer.cpp

@@ -164,7 +164,7 @@ static void attachSystemTables(const Context & context)
     {
         /// TODO: add attachTableDelayed into DatabaseMemory to speedup loading
         system_database = std::make_shared<DatabaseMemory>(DatabaseCatalog::SYSTEM_DATABASE, context);
-        DatabaseCatalog::instance().attachDatabase(DatabaseCatalog::SYSTEM_DATABASE, system_database);
+        DatabaseCatalog::instance().attachDatabase(DatabaseCatalog::SYSTEM_DATABASE, UUIDHelpers::Nil,  system_database);
     }
 
     attachSystemTablesLocal(*system_database);
@@ -241,7 +241,7 @@ try
       *  if such tables will not be dropped, clickhouse-server will not be able to load them due to security reasons.
       */
     std::string default_database = config().getString("default_database", "_local");
-    DatabaseCatalog::instance().attachDatabase(default_database, std::make_shared<DatabaseMemory>(default_database, *context));
+    DatabaseCatalog::instance().attachDatabase(default_database, UUIDHelpers::Nil, std::make_shared<DatabaseMemory>(default_database, *context));
     context->setCurrentDatabase(default_database);
     applyCmdOptions();
 

src/Databases/DatabaseAtomic.cpp

@@ -102,7 +102,6 @@ void DatabaseAtomic::attachTable(const String & name, const StoragePtr & table,
     assertDetachedTableNotInUse(table->getStorageID().uuid);
     DatabaseWithDictionaries::attachTableUnlocked(name, table, lock);
     table_name_to_path.emplace(std::make_pair(name, relative_table_path));
-    //tryCreateSymlink(name, relative_table_path);
 }
 
 StoragePtr DatabaseAtomic::detachTable(const String & name)
@@ -113,7 +112,6 @@ StoragePtr DatabaseAtomic::detachTable(const String & name)
     table_name_to_path.erase(name);
     detached_tables.emplace(table->getStorageID().uuid, table);
     not_in_use = cleenupDetachedTables();
-    //tryRemoveSymlink(name);
     return table;
 }
 
@@ -360,7 +358,7 @@ void DatabaseAtomic::tryCreateSymlink(const String & table_name, const String &
     }
     catch (...)
     {
-        tryLogCurrentException(log);
+        LOG_WARNING(log, getCurrentExceptionMessage(true));
     }
 }
 
@@ -373,7 +371,7 @@ void DatabaseAtomic::tryRemoveSymlink(const String & table_name)
     }
     catch (...)
     {
-        tryLogCurrentException(log);
+        LOG_WARNING(log, getCurrentExceptionMessage(true));
     }
 }
 

src/Databases/DatabaseAtomic.h

@@ -52,6 +52,9 @@ public:
 
     UUID tryGetTableUUID(const String & table_name) const override;
 
+    void tryCreateSymlink(const String & table_name, const String & actual_data_path);
+    void tryRemoveSymlink(const String & table_name);
+
 private:
     void commitAlterTable(const StorageID & table_id, const String & table_metadata_tmp_path, const String & table_metadata_path) override;
     void commitCreateTable(const ASTCreateQuery & query, const StoragePtr & table,
@@ -61,9 +64,6 @@ private:
     typedef std::unordered_map<UUID, StoragePtr> DetachedTables;
     [[nodiscard]] DetachedTables cleenupDetachedTables();
 
-    void tryCreateSymlink(const String & table_name, const String & actual_data_path);
-    void tryRemoveSymlink(const String & table_name);
-
     //TODO store path in DatabaseWithOwnTables::tables
     typedef std::unordered_map<String, String> NameToPathMap;
     NameToPathMap table_name_to_path;

src/Databases/DatabaseOnDisk.cpp

@@ -20,6 +20,7 @@
 
 #include <Databases/DatabaseOrdinary.h>
 #include <Databases/DatabaseAtomic.h>
+#include <Common/assert_cast.h>
 
 
 namespace DB
@@ -305,6 +306,14 @@ void DatabaseOnDisk::renameTable(
     to_database.createTable(context, to_table_name, table, attach_query);
 
     Poco::File(table_metadata_path).remove();
+
+    /// Special case: usually no actions with symlinks are required when detaching/attaching table,
+    /// but not when moving from Atomic database to Ordinary
+    if (from_atomic_to_ordinary)
+    {
+        auto & atomic_db = assert_cast<DatabaseAtomic &>(*this);
+        atomic_db.tryRemoveSymlink(table_name);
+    }
 }
 
 ASTPtr DatabaseOnDisk::getCreateTableQueryImpl(const String & table_name, const Context &, bool throw_on_error) const
@@ -444,7 +453,8 @@ ASTPtr DatabaseOnDisk::parseQueryFromMetadata(Poco::Logger * logger, const Conte
       */
     if (remove_empty && query.empty())
     {
-        LOG_ERROR(logger, "File {} is empty. Removing.", metadata_file_path);
+        if (logger)
+            LOG_ERROR(logger, "File {} is empty. Removing.", metadata_file_path);
         Poco::File(metadata_file_path).remove();
         return nullptr;
     }
@@ -467,7 +477,7 @@ ASTPtr DatabaseOnDisk::parseQueryFromMetadata(Poco::Logger * logger, const Conte
         String table_name = Poco::Path(metadata_file_path).makeFile().getBaseName();
         table_name = unescapeForFileName(table_name);
 
-        if (create.table != TABLE_WITH_UUID_NAME_PLACEHOLDER)
+        if (create.table != TABLE_WITH_UUID_NAME_PLACEHOLDER && logger)
             LOG_WARNING(logger, "File {} contains both UUID and table name. Will use name `{}` instead of `{}`", metadata_file_path, table_name, create.table);
         create.table = table_name;
     }

src/Interpreters/DatabaseCatalog.cpp

@@ -114,7 +114,7 @@ void DatabaseCatalog::loadDatabases()
     drop_delay_sec = global_context->getConfigRef().getInt("database_atomic_delay_before_drop_table_sec", default_drop_delay_sec);
 
     auto db_for_temporary_and_external_tables = std::make_shared<DatabaseMemory>(TEMPORARY_DATABASE, *global_context);
-    attachDatabase(TEMPORARY_DATABASE, db_for_temporary_and_external_tables);
+    attachDatabase(TEMPORARY_DATABASE, UUIDHelpers::Nil, db_for_temporary_and_external_tables);
 
     loadMarkedAsDroppedTables();
     auto task_holder = global_context->getSchedulePool().createTask("DatabaseCatalog", [this](){ this->dropTableDataTask(); });
@@ -147,6 +147,7 @@ void DatabaseCatalog::shutdownImpl()
     std::lock_guard lock(databases_mutex);
     assert(std::find_if_not(uuid_map.begin(), uuid_map.end(), [](const auto & elem) { return elem.map.empty(); }) == uuid_map.end());
     databases.clear();
+    db_uuid_map.clear();
     view_dependencies.clear();
 }
 
@@ -246,11 +247,13 @@ void DatabaseCatalog::assertDatabaseDoesntExistUnlocked(const String & database_
         throw Exception("Database " + backQuoteIfNeed(database_name) + " already exists.", ErrorCodes::DATABASE_ALREADY_EXISTS);
 }
 
-void DatabaseCatalog::attachDatabase(const String & database_name, const DatabasePtr & database)
+void DatabaseCatalog::attachDatabase(const String & database_name, const UUID & uuid, const DatabasePtr & database)
 {
     std::lock_guard lock{databases_mutex};
     assertDatabaseDoesntExistUnlocked(database_name);
-    databases[database_name] = database;
+    databases.emplace(database_name, database);
+    if (uuid != UUIDHelpers::Nil)
+        db_uuid_map.emplace(uuid, database);
 }
 
 
@@ -275,6 +278,7 @@ DatabasePtr DatabaseCatalog::detachDatabase(const String & database_name, bool d
                 database_atomic->assertCanBeDetached(false);
         }
 
+        db_uuid_map.erase(db->getUUID());
         databases.erase(database_name);
     }
 
@@ -312,6 +316,25 @@ DatabasePtr DatabaseCatalog::tryGetDatabase(const String & database_name) const
     return it->second;
 }
 
+DatabasePtr DatabaseCatalog::getDatabase(const UUID & uuid) const
+{
+    std::lock_guard lock{databases_mutex};
+    auto it = db_uuid_map.find(uuid);
+    if (it == db_uuid_map.end())
+        throw Exception(ErrorCodes::UNKNOWN_DATABASE, "Database UUID {} does not exist", toString(uuid));
+    return it->second;
+}
+
+DatabasePtr DatabaseCatalog::tryGetDatabase(const UUID & uuid) const
+{
+    assert(uuid != UUIDHelpers::Nil);
+    std::lock_guard lock{databases_mutex};
+    auto it = db_uuid_map.find(uuid);
+    if (it == db_uuid_map.end())
+        return {};
+    return it->second;
+}
+
 bool DatabaseCatalog::isDatabaseExist(const String & database_name) const
 {
     assert(!database_name.empty());

src/Interpreters/DatabaseCatalog.h

@@ -121,12 +121,14 @@ public:
     DatabasePtr getDatabaseForTemporaryTables() const;
     DatabasePtr getSystemDatabase() const;
 
-    void attachDatabase(const String & database_name, const DatabasePtr & database);
+    void attachDatabase(const String & database_name, const UUID & uuid, const DatabasePtr & database);
     DatabasePtr detachDatabase(const String & database_name, bool drop = false, bool check_empty = true);
 
     /// database_name must be not empty
     DatabasePtr getDatabase(const String & database_name) const;
     DatabasePtr tryGetDatabase(const String & database_name) const;
+    DatabasePtr getDatabase(const UUID & uuid) const;
+    DatabasePtr tryGetDatabase(const UUID & uuid) const;
     bool isDatabaseExist(const String & database_name) const;
     Databases getDatabases() const;
 
@@ -211,6 +213,8 @@ private:
     static constexpr size_t reschedule_time_ms = 100;
 
 private:
+    using UUIDToDatabaseMap = std::unordered_map<UUID, DatabasePtr>;
+
     /// For some reason Context is required to get Storage from Database object
     Context * global_context;
     mutable std::mutex databases_mutex;
@@ -218,6 +222,7 @@ private:
     ViewDependencies view_dependencies;
 
     Databases databases;
+    UUIDToDatabaseMap db_uuid_map;
     UUIDToStorageMap uuid_map;
 
     Poco::Logger * log;

src/Interpreters/InterpreterCreateQuery.cpp

@@ -47,6 +47,7 @@
 
 #include <Databases/DatabaseFactory.h>
 #include <Databases/IDatabase.h>
+#include <Databases/DatabaseOnDisk.h>
 
 #include <Dictionaries/getDictionaryConfigurationFromAST.h>
 
@@ -87,6 +88,7 @@ InterpreterCreateQuery::InterpreterCreateQuery(const ASTPtr & query_ptr_, Contex
 BlockIO InterpreterCreateQuery::createDatabase(ASTCreateQuery & create)
 {
     String database_name = create.database;
+    assert(database_name != TABLE_WITH_UUID_NAME_PLACEHOLDER);
 
     auto guard = DatabaseCatalog::instance().getDDLGuard(database_name, "");
 
@@ -99,7 +101,26 @@ BlockIO InterpreterCreateQuery::createDatabase(ASTCreateQuery & create)
             throw Exception("Database " + database_name + " already exists.", ErrorCodes::DATABASE_ALREADY_EXISTS);
     }
 
-    if (!create.storage)
+
+    /// Will write file with database metadata, if needed.
+    String database_name_escaped = escapeForFileName(database_name);
+    fs::path metadata_path = fs::canonical(context.getPath());
+    fs::path metadata_file_tmp_path = metadata_path / "metadata" / (database_name_escaped + ".sql.tmp");
+    fs::path metadata_file_path = metadata_path / "metadata" / (database_name_escaped + ".sql");
+
+    if (!create.storage && create.attach)
+    {
+        if (!fs::exists(metadata_file_path))
+            throw Exception("Database engine must be specified for ATTACH DATABASE query", ErrorCodes::UNKNOWN_DATABASE_ENGINE);
+        /// Short syntax: try read database definition from file
+        auto ast = DatabaseOnDisk::parseQueryFromMetadata(nullptr, context, metadata_file_path);
+        create = ast->as<ASTCreateQuery &>();
+        if (!create.table.empty() || !create.storage)
+            throw Exception(ErrorCodes::INCORRECT_QUERY, "Metadata file {} contains incorrect CREATE DATABASE query", metadata_file_path);
+        create.attach = true;
+        create.attach_short_syntax = true;
+    }
+    else if (!create.storage)
     {
         /// For new-style databases engine is explicitly specified in .sql
         /// When attaching old-style database during server startup, we must always use Ordinary engine
@@ -120,12 +141,6 @@ BlockIO InterpreterCreateQuery::createDatabase(ASTCreateQuery & create)
         throw Exception("Unknown database engine: " + ostr.str(), ErrorCodes::UNKNOWN_DATABASE_ENGINE);
     }
 
-    /// Will write file with database metadata, if needed.
-    String database_name_escaped = escapeForFileName(database_name);
-    fs::path metadata_path = fs::canonical(context.getPath());
-    fs::path metadata_file_tmp_path = metadata_path / "metadata" / (database_name_escaped + ".sql.tmp");
-    fs::path metadata_file_path = metadata_path / "metadata" / (database_name_escaped + ".sql");
-
     if (create.storage->engine->name == "Atomic")
     {
         if (!context.getSettingsRef().allow_experimental_database_atomic && !internal)
@@ -152,7 +167,8 @@ BlockIO InterpreterCreateQuery::createDatabase(ASTCreateQuery & create)
         metadata_path = metadata_path / "metadata" / database_name_escaped;
     }
 
-    DatabasePtr database = DatabaseFactory::get(database_name, metadata_path, create.storage, create.uuid, context);
+    // FIXME
+    DatabasePtr database = DatabaseFactory::get(database_name, metadata_path / "", create.storage, create.uuid, context);
 
     bool need_write_metadata = !create.attach || !fs::exists(metadata_file_path);
 
@@ -181,7 +197,7 @@ BlockIO InterpreterCreateQuery::createDatabase(ASTCreateQuery & create)
     try
     {
         /// TODO Attach db only after it was loaded. Now it's not possible because of view dependencies
-        DatabaseCatalog::instance().attachDatabase(database_name, database);
+        DatabaseCatalog::instance().attachDatabase(database_name, create.uuid, database);
         added = true;
 
         if (need_write_metadata)

src/Interpreters/loadMetadata.cpp

@@ -156,7 +156,7 @@ void loadMetadataSystem(Context & context)
         Poco::File(global_path + "metadata/" SYSTEM_DATABASE).createDirectories();
 
         auto system_database = std::make_shared<DatabaseOrdinary>(SYSTEM_DATABASE, global_path + "metadata/" SYSTEM_DATABASE "/", context);
-        DatabaseCatalog::instance().attachDatabase(SYSTEM_DATABASE, system_database);
+        DatabaseCatalog::instance().attachDatabase(SYSTEM_DATABASE, UUIDHelpers::Nil, system_database);
     }
 
 }

src/Interpreters/tests/in_join_subqueries_preprocessor.cpp

@@ -1167,7 +1167,7 @@ TestResult check(const TestEntry & entry)
         auto storage_distributed_hits = StorageDistributedFake::create("distant_db", "distant_hits", entry.shard_count);
 
         DB::DatabasePtr database = std::make_shared<DB::DatabaseOrdinary>("test", "./metadata/test/", context);
-        DB::DatabaseCatalog::instance().attachDatabase("test", database);
+        DB::DatabaseCatalog::instance().attachDatabase("test", UUIDHelpers::Nil, database);
         database->attachTable("visits_all", storage_distributed_visits);
         database->attachTable("hits_all", storage_distributed_hits);
         context.setCurrentDatabase("test");

src/Storages/tests/gtest_transform_query_for_external_database.cpp

@@ -42,7 +42,7 @@ private:
         registerFunctions();
         DatabasePtr database = std::make_shared<DatabaseMemory>("test", context);
         database->attachTable("table", StorageMemory::create(StorageID("test", "table"), ColumnsDescription{columns}, ConstraintsDescription{}));
-        DatabaseCatalog::instance().attachDatabase("test", database);
+        DatabaseCatalog::instance().attachDatabase("test", UUIDHelpers::Nil, database);
         context.setCurrentDatabase("test");
     }
 };

tests/queries/0_stateless/01107_atomic_db_detach_attach.sh

@@ -18,7 +18,7 @@ wait
 $CLICKHOUSE_CLIENT -q "ATTACH TABLE test_01107.mt"
 $CLICKHOUSE_CLIENT -q "SELECT count(n), sum(n) FROM test_01107.mt"
 $CLICKHOUSE_CLIENT -q "DETACH DATABASE test_01107"
-$CLICKHOUSE_CLIENT --allow_experimental_database_atomic=1 -q "ATTACH DATABASE test_01107 ENGINE=Atomic"
+$CLICKHOUSE_CLIENT --allow_experimental_database_atomic=1 -q "ATTACH DATABASE test_01107"
 $CLICKHOUSE_CLIENT -q "SELECT count(n), sum(n) FROM test_01107.mt"
 
 $CLICKHOUSE_CLIENT -q "INSERT INTO test_01107.mt SELECT number + sleepEachRow(1) FROM numbers(5)" && echo "end" &

tests/queries/0_stateless/01114_database_atomic.sh

@@ -12,8 +12,8 @@ $CLICKHOUSE_CLIENT --allow_experimental_database_atomic=1 -q "CREATE DATABASE te
 $CLICKHOUSE_CLIENT --default_database_engine=Atomic --allow_experimental_database_atomic=1 -q "CREATE DATABASE test_01114_2"
 $CLICKHOUSE_CLIENT --default_database_engine=Ordinary -q "CREATE DATABASE test_01114_3"
 
-$CLICKHOUSE_CLIENT -q "SHOW CREATE DATABASE test_01114_1"
-$CLICKHOUSE_CLIENT -q "SHOW CREATE DATABASE test_01114_2"
+$CLICKHOUSE_CLIENT --show_table_uuid_in_table_create_query_if_not_nil=0 -q "SHOW CREATE DATABASE test_01114_1"
+$CLICKHOUSE_CLIENT --show_table_uuid_in_table_create_query_if_not_nil=0 -q "SHOW CREATE DATABASE test_01114_2"
 $CLICKHOUSE_CLIENT -q "SHOW CREATE DATABASE test_01114_3"
 $CLICKHOUSE_CLIENT -q "SELECT name, engine, splitByChar('/', data_path)[-2], splitByChar('/', metadata_path)[-3], splitByChar('/', metadata_path)[-2] FROM system.databases WHERE name LIKE 'test_01114_%'"