SYSTEM RELOAD/UNLOAD DICTIONARY ON CLUSTER ignores the initiator's current database and silently targets default #114322

Company or project name

ClickHouse CI monitoring (@groeneai).

Describe what's wrong

SYSTEM RELOAD DICTIONARY <bare-name> ON CLUSTER <c> and SYSTEM UNLOAD DICTIONARY <bare-name> ON CLUSTER <c> do not resolve the dictionary name against the initiator's current database before the statement is put on the DDL queue. The bare name is serialized as-is, so every worker re-resolves it against its own current database, which for a query arriving over the DDL queue is default.

Consequence: run from database mydb, the command silently reloads/unloads default.d instead of mydb.d, and it returns success. If default.d does not exist the command is a no-op; if it does exist, an unrelated dictionary is reloaded. There is no error either way, so the operator has no signal that the wrong dictionary was touched.

Filed at the request of @Manerone in #111756, which fixes part of this. See "Additional context" for what that PR covers and what remains.

Does it reproduce on the most recent release?

Yes

How to reproduce

Version: master 26.8.1.1 (commit c462bd70270441b71070b4c5417a32e33cac0bab), debug build. No non-default settings.

Single node is enough. The server needs <distributed_ddl>, a keeper, and a cluster containing itself:

<clickhouse>
    <distributed_ddl><path>/clickhouse/task_queue/ddl</path></distributed_ddl>
    <remote_servers>
        <one_node><shard><replica><host>127.0.0.1</host><port>9000</port></replica></shard></one_node>
    </remote_servers>
</clickhouse>

CREATE DATABASE mydb;

-- the dictionary the user means
CREATE OR REPLACE DICTIONARY mydb.d (k UInt64, v String)
PRIMARY KEY k SOURCE(NULL()) LAYOUT(FLAT()) LIFETIME(0);

-- a same-named decoy in `default`
CREATE OR REPLACE DICTIONARY default.d (k UInt64, v String)
PRIMARY KEY k SOURCE(NULL()) LAYOUT(FLAT()) LIFETIME(0);

SYSTEM UNLOAD DICTIONARY mydb.d;
SYSTEM UNLOAD DICTIONARY default.d;

USE mydb;
SYSTEM RELOAD DICTIONARY d ON CLUSTER one_node;

SELECT database, name, status FROM system.dictionaries WHERE name = 'd' ORDER BY database;

Expected behavior

mydb.d is LOADED, default.d stays NOT_LOADED -- the same target the non-ON CLUSTER form picks.

Actual result

after ON CLUSTER          default  d  LOADED
after ON CLUSTER          mydb     d  NOT_LOADED

Control, same session, same server, ON CLUSTER dropped:

after local (control)     default  d  NOT_LOADED
after local (control)     mydb     d  LOADED

The statement returned success in the failing case. The entry serialized into Keeper carries the unqualified name:

query: SYSTEM RELOAD DICTIONARY ON CLUSTER one_node d

SYSTEM UNLOAD DICTIONARY ... ON CLUSTER behaves the same way.

Additional context

Code path on master (a1ae499e22944d8):

• InterpreterSystemQuery::execute returns inside the if (!query.cluster.empty()) branch (src/Interpreters/InterpreterSystemQuery.cpp:361-366) before the RELOAD_DICTIONARY / UNLOAD_DICTIONARY canonicalization block at :378-382, so on the ON CLUSTER path no qualification happens at all.
• The worker-side rewrite does not supply it either: ASTSystemQuery::getRewrittenASTWithoutOnCluster (src/Parsers/ASTSystemQuery.h:278-281) calls removeOnCluster (src/Parsers/ASTQueryWithOnCluster.h:48-57), which injects params.default_database. DDLTask.cpp:507 sets that from address_in_cluster.default_database, i.e. the cluster address's <default_database>, not the initiator's current database.

PR #111756 by @Manerone moves the qualification ahead of the ON CLUSTER dispatch and fixes the case above. I verified that empirically: with only its InterpreterSystemQuery.cpp hunk applied to a1ae499e22944d8 (build id a9c961304b95de468e98629a04ada7c56003c136), an ON CLUSTER command issued from a node that is itself in the cluster and hosts mydb.d leaves mydb.d LOADED and default.d NOT_LOADED.

One case is still open with that patch applied, which is why the PR was left unmerged. ExternalDictionariesLoader::qualifyDictionaryNameWithDatabase (src/Interpreters/ExternalDictionariesLoader.cpp:211-232) only fills in .database when the calling server can resolve the name itself (has(resolved_name)). So a coordinator or gateway node that enqueues the DDL without hosting the dictionary leaves the name bare and each worker falls back to default again.

Measured on a two-node setup, both nodes running the patched binary: node A is the initiator and is not a member of cluster c_workers and hosts no dictionaries; node B is the only cluster member and hosts both mydb.d and default.d.

-- on node A, in database mydb:
SYSTEM RELOAD DICTIONARY d ON CLUSTER c_workers;    -- returns success

-- observed on node B:
default  d  LOADED
mydb     d  NOT_LOADED

Keeper entry: query: SYSTEM RELOAD DICTIONARY ON CLUSTER c_workers d.

The precondition is confirmed on node A: USE mydb; SYSTEM RELOAD DICTIONARY d throws Code: 36. DB::Exception: Dictionary (\d`) not found. (BAD_ARGUMENTS)andsystem.dictionariesthere is empty, so thehas()` check takes the leave-bare path.

Fixing this seems to need the initiator's current database propagated independently of whether the initiator can resolve the dictionary locally. That has one wrinkle worth deciding explicitly: for a bare name, local execution resolves an XML dictionary ahead of a same-named dictionary in the current database, so unconditionally qualifying would change which one a bare ON CLUSTER name selects on workers that host an XML dictionary of that name.