From 66d2e274341c3af69189a727f919ab19eac68560 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Thu, 15 Oct 2026 23:49:31 +0000 Subject: [PATCH 1/2] Register XML dictionaries before loading tables at startup The definitions of the dictionaries from the configuration files were added to `ExternalDictionariesLoader` only after `loadMetadata`, so a table referring to such a dictionary was attached while the dictionary was unknown: the dependency was dropped by `TablesLoader::removeUnresolvableDependencies` as nonexistent, and a projection using `dictGet` could not be analyzed and was skipped. Now the configuration repository is registered before `loadMetadata`, while automatic loading is disabled in the loader until `loadOrReloadDictionaries`, so no dictionary starts loading earlier than before. Closes: https://github.com/ClickHouse/ClickHouse/issues/67521 Co-Authored-By: Claude Opus 5.5 --- programs/server/Server.cpp | 3 ++ src/Interpreters/Context.cpp | 25 ++++++++++++- src/Interpreters/Context.h | 3 ++ src/Interpreters/ExternalLoader.cpp | 37 +++++++++++++++---- src/Interpreters/ExternalLoader.h | 4 ++ .../configs/no_async_load_databases.xml | 4 ++ .../test_dictionaries_dependency_xml/test.py | 27 +++++++++++++- 7 files changed, 93 insertions(+), 10 deletions(-) create mode 100644 tests/integration/test_dictionaries_dependency_xml/configs/no_async_load_databases.xml diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index 9057acdaec5f..c0e826337675 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -3625,6 +3625,9 @@ try /// and so loadMarkedAsDroppedTables() will find it and try to add, and UUID will overlap. database_catalog.loadMarkedAsDroppedTables(); database_catalog.createBackgroundTasks(); + /// The tables can depend on the dictionaries from the configuration files, so register them before + /// loading the databases. They start loading later, in `loadOrReloadDictionaries`. + global_context->registerDictionariesBeforeLoadingMetadata(config()); /// Then, load remaining databases (some of them maybe be loaded asynchronously) load_metadata_tasks = loadMetadata(global_context, default_database, server_settings[ServerSetting::async_load_databases]); /// If we need to convert database engines, disable async tables loading diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 6ab90491b702..5d1c205f60e6 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -4553,16 +4553,37 @@ void Context::tryCreateEmbeddedDictionaries() const static_cast(getEmbeddedDictionariesImpl(true)); } +void Context::registerDictionariesBeforeLoadingMetadata(const Poco::Util::AbstractConfiguration & config) +{ + std::lock_guard lock(shared->external_dictionaries_mutex); + + /// The tables are attached before the dictionaries are allowed to load, but they can refer to dictionaries + /// declared in configuration files: in a projection, in a column default, or in their loading dependencies. + /// So the definitions are registered now, while no dictionary starts loading until `loadOrReloadDictionaries`: + /// a dictionary may read from a table which is not attached yet, or from this server, which does not listen yet. + auto & external_dictionaries_loader = getExternalDictionariesLoaderWithLock(lock); + external_dictionaries_loader.enableAutomaticLoading(false); + addDictionariesConfigRepository(config, lock); +} + void Context::loadOrReloadDictionaries(const Poco::Util::AbstractConfiguration & config) { bool dictionaries_lazy_load = shared->server_settings[ServerSetting::dictionaries_lazy_load]; - auto patterns_values = getMultipleValuesFromConfig(config, "", "dictionaries_config"); - std::unordered_set patterns(patterns_values.begin(), patterns_values.end()); std::lock_guard lock(shared->external_dictionaries_mutex); auto & external_dictionaries_loader = getExternalDictionariesLoaderWithLock(lock); external_dictionaries_loader.enableAlwaysLoadEverything(!dictionaries_lazy_load); + addDictionariesConfigRepository(config, lock); + external_dictionaries_loader.enableAutomaticLoading(true); +} + +void Context::addDictionariesConfigRepository(const Poco::Util::AbstractConfiguration & config, const std::lock_guard & lock) TSA_REQUIRES(shared->external_dictionaries_mutex) +{ + auto patterns_values = getMultipleValuesFromConfig(config, "", "dictionaries_config"); + std::unordered_set patterns(patterns_values.begin(), patterns_values.end()); + + auto & external_dictionaries_loader = getExternalDictionariesLoaderWithLock(lock); if (shared->external_dictionaries_config_repository) { diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index 3196a40b2bf3..0b67b74601e2 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -1351,6 +1351,8 @@ public: const EmbeddedDictionaries & getEmbeddedDictionaries() const; EmbeddedDictionaries & getEmbeddedDictionaries(); void tryCreateEmbeddedDictionaries() const; + /// Registers the dictionaries from the configuration files without loading them, so the tables referring to them can be attached. + void registerDictionariesBeforeLoadingMetadata(const Poco::Util::AbstractConfiguration & config); void loadOrReloadDictionaries(const Poco::Util::AbstractConfiguration & config); void waitForDictionariesLoad() const; @@ -2239,6 +2241,7 @@ private: void checkMergeTreeSettingsConstraintsWithLock(const MergeTreeSettings & merge_tree_settings, const SettingsChanges & changes) const; ExternalDictionariesLoader & getExternalDictionariesLoaderWithLock(const std::lock_guard & lock); + void addDictionariesConfigRepository(const Poco::Util::AbstractConfiguration & config, const std::lock_guard & lock); ExternalUserDefinedExecutableFunctionsLoader & getExternalUserDefinedExecutableFunctionsLoaderWithLock(const std::lock_guard & lock); diff --git a/src/Interpreters/ExternalLoader.cpp b/src/Interpreters/ExternalLoader.cpp index c89749fd124e..01fbae4f6ca2 100644 --- a/src/Interpreters/ExternalLoader.cpp +++ b/src/Interpreters/ExternalLoader.cpp @@ -495,7 +495,7 @@ public: if (!infos.contains(name)) { Info & info = infos.emplace(name, Info{name, config}).first->second; - if (!isObjectLazy(*config)) + if (automatic_loading_enabled && !isObjectLazy(*config)) { LOG_TRACE(log, "Will load '{}' because it is not loaded lazily.", name); startLoading(info); @@ -530,13 +530,23 @@ public: always_load_everything = enable; + if (enable && automatic_loading_enabled) + startLoadingNotLazy(); + } + + /// Sets whether the objects which are not loaded lazily start loading on their own, when they are added + /// to the configuration. When disabled, the objects are registered and can be found, but they are loaded + /// only when requested explicitly. When enabled again, the loading of such objects is started. + void enableAutomaticLoading(bool enable) + { + std::lock_guard lock{mutex}; + if (automatic_loading_enabled == enable) + return; + + automatic_loading_enabled = enable; + if (enable) - { - /// Start loading all the objects which were not loaded yet and are not configured to load lazily. - for (auto & [name, info] : infos) - if (!info.triedToLoad() && !isObjectLazy(*info.config)) - startLoading(info); - } + startLoadingNotLazy(); } /// Sets whether the objects should be loaded asynchronously, each loading in a new thread (from the thread pool). @@ -769,6 +779,13 @@ public: } private: + /// Starts loading all the objects which were not loaded yet and are not configured to load lazily. + void startLoadingNotLazy() + { + for (auto & [name, info] : infos) + if (!info.triedToLoad() && !isObjectLazy(*info.config)) + startLoading(info); + } struct Info { @@ -1323,6 +1340,7 @@ private: ObjectConfigsPtr configs; std::unordered_map infos; std::atomic always_load_everything = false; + std::atomic automatic_loading_enabled = true; std::atomic enable_async_loading = false; std::unordered_map loading_threads; std::vector recently_finished_loadings; @@ -1466,6 +1484,11 @@ void ExternalLoader::enableAlwaysLoadEverything(bool enable) loading_dispatcher->enableAlwaysLoadEverything(enable); } +void ExternalLoader::enableAutomaticLoading(bool enable) +{ + loading_dispatcher->enableAutomaticLoading(enable); +} + void ExternalLoader::enableAsyncLoading(bool enable) { loading_dispatcher->enableAsyncLoading(enable); diff --git a/src/Interpreters/ExternalLoader.h b/src/Interpreters/ExternalLoader.h index 10b3cd145844..82561887e912 100644 --- a/src/Interpreters/ExternalLoader.h +++ b/src/Interpreters/ExternalLoader.h @@ -100,6 +100,10 @@ public: /// Sets whether all the objects from the configuration should be always loaded (even those which are never used). void enableAlwaysLoadEverything(bool enable); + /// Sets whether the objects which are not loaded lazily start loading on their own when they are added to the configuration. + /// When disabled, the objects are registered, but loaded only on explicit request. + void enableAutomaticLoading(bool enable); + /// Sets whether the objects should be loaded asynchronously, each loading in a new thread (from the thread pool). void enableAsyncLoading(bool enable); diff --git a/tests/integration/test_dictionaries_dependency_xml/configs/no_async_load_databases.xml b/tests/integration/test_dictionaries_dependency_xml/configs/no_async_load_databases.xml new file mode 100644 index 000000000000..6331a8f1c063 --- /dev/null +++ b/tests/integration/test_dictionaries_dependency_xml/configs/no_async_load_databases.xml @@ -0,0 +1,4 @@ + + + false + diff --git a/tests/integration/test_dictionaries_dependency_xml/test.py b/tests/integration/test_dictionaries_dependency_xml/test.py index bf587e88cdd6..45bce1efe21e 100644 --- a/tests/integration/test_dictionaries_dependency_xml/test.py +++ b/tests/integration/test_dictionaries_dependency_xml/test.py @@ -12,7 +12,10 @@ DICTIONARY_FILES = [ cluster = ClickHouseCluster(__file__) instance = cluster.add_instance( - "instance", dictionaries=DICTIONARY_FILES, stay_alive=True + "instance", + main_configs=["configs/no_async_load_databases.xml"], + dictionaries=DICTIONARY_FILES, + stay_alive=True, ) @@ -180,3 +183,25 @@ def test_xml_dict_same_name(started_cluster): instance.restart_clickhouse() assert "node" in instance.query("show tables from default") instance.query("drop table default.node") + + +def test_projection_with_xml_dict(started_cluster): + # The dictionaries from the configuration files have to be known when the tables are attached at startup, + # otherwise the projection cannot be analyzed and is skipped. + instance.query("DROP TABLE IF EXISTS default.prj") + instance.query( + "CREATE TABLE default.prj (x UInt64, " + "PROJECTION p (SELECT x, dictGet('node', 'name', x % 2) AS v GROUP BY x, v)) " + "ENGINE = MergeTree ORDER BY x" + ) + instance.query("INSERT INTO default.prj SELECT number FROM numbers(10)") + instance.restart_clickhouse() + assert ( + instance.query( + "SELECT name FROM system.projections WHERE database = 'default' AND table = 'prj'" + ) + == "p\n" + ) + instance.query("ALTER TABLE default.prj ADD COLUMN y UInt64") + assert instance.query("SELECT count() FROM default.prj") == "10\n" + instance.query("DROP TABLE default.prj") From 7ba365178681b43ae37b9664904b61d3007d3865 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 17 Oct 2026 10:09:03 +0000 Subject: [PATCH 2/2] Address review: register XML dictionaries before loading the `system` database `loadMetadataSystem` attaches the persisted tables of the `system` database before the other databases, so a table in `system` with a projection using `dictGet` on an XML dictionary was still attached while the dictionary was unknown. Register the dictionaries right after the temporary database is loaded, and run the regression test for a table in `system` too. Co-Authored-By: Claude Opus 5.5 --- programs/server/Server.cpp | 6 +++--- .../test_dictionaries_dependency_xml/test.py | 20 ++++++++++--------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index c0e826337675..e817adafb98c 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -3588,6 +3588,9 @@ try auto & database_catalog = DatabaseCatalog::instance(); /// We load temporary database first, because projections need it. database_catalog.initializeAndLoadTemporaryDatabase(); + /// The tables (including the ones in the `system` database) can depend on the dictionaries from the configuration files, + /// so register them before loading the databases. They start loading later, in `loadOrReloadDictionaries`. + global_context->registerDictionariesBeforeLoadingMetadata(config()); load_system_metadata_tasks = loadMetadataSystem(global_context, server_settings[ServerSetting::async_load_system_database]); maybeConvertSystemDatabase(global_context, load_system_metadata_tasks); @@ -3625,9 +3628,6 @@ try /// and so loadMarkedAsDroppedTables() will find it and try to add, and UUID will overlap. database_catalog.loadMarkedAsDroppedTables(); database_catalog.createBackgroundTasks(); - /// The tables can depend on the dictionaries from the configuration files, so register them before - /// loading the databases. They start loading later, in `loadOrReloadDictionaries`. - global_context->registerDictionariesBeforeLoadingMetadata(config()); /// Then, load remaining databases (some of them maybe be loaded asynchronously) load_metadata_tasks = loadMetadata(global_context, default_database, server_settings[ServerSetting::async_load_databases]); /// If we need to convert database engines, disable async tables loading diff --git a/tests/integration/test_dictionaries_dependency_xml/test.py b/tests/integration/test_dictionaries_dependency_xml/test.py index 45bce1efe21e..202cd63b6de0 100644 --- a/tests/integration/test_dictionaries_dependency_xml/test.py +++ b/tests/integration/test_dictionaries_dependency_xml/test.py @@ -185,23 +185,25 @@ def test_xml_dict_same_name(started_cluster): instance.query("drop table default.node") -def test_projection_with_xml_dict(started_cluster): +@pytest.mark.parametrize("database", ["default", "system"]) +def test_projection_with_xml_dict(started_cluster, database): # The dictionaries from the configuration files have to be known when the tables are attached at startup, - # otherwise the projection cannot be analyzed and is skipped. - instance.query("DROP TABLE IF EXISTS default.prj") + # otherwise the projection cannot be analyzed and is skipped. The `system` database is loaded before the others. + table = f"{database}.prj" + instance.query(f"DROP TABLE IF EXISTS {table}") instance.query( - "CREATE TABLE default.prj (x UInt64, " + f"CREATE TABLE {table} (x UInt64, " "PROJECTION p (SELECT x, dictGet('node', 'name', x % 2) AS v GROUP BY x, v)) " "ENGINE = MergeTree ORDER BY x" ) - instance.query("INSERT INTO default.prj SELECT number FROM numbers(10)") + instance.query(f"INSERT INTO {table} SELECT number FROM numbers(10)") instance.restart_clickhouse() assert ( instance.query( - "SELECT name FROM system.projections WHERE database = 'default' AND table = 'prj'" + f"SELECT name FROM system.projections WHERE database = '{database}' AND table = 'prj'" ) == "p\n" ) - instance.query("ALTER TABLE default.prj ADD COLUMN y UInt64") - assert instance.query("SELECT count() FROM default.prj") == "10\n" - instance.query("DROP TABLE default.prj") + instance.query(f"ALTER TABLE {table} ADD COLUMN y UInt64") + assert instance.query(f"SELECT count() FROM {table}") == "10\n" + instance.query(f"DROP TABLE {table}")