All committers have signed the CLA.

This needs more thought.

1. The behavior will conflict with the Replicated database.
2. The names are quite convoluted, we need something better, but at the same time, consistent with cluster_for_parallel_replicas.

2. convoluted

Thanks a lot, I will further optimize these two points later. Then I will add a test case

Workflow [PR], commit [6548354]

Summary: ❌

AI Review

Summary

This PR adds an experimental setting pair that rewrites eligible initial queries with a missing ON CLUSTER to a configured default cluster. The implementation now covers many earlier edge cases, and the current Praktika report shows no failed jobs, but there are still unresolved classification gaps where auto-fill can either leave eligible statements local or distribute access-storage operations that are not safe to run on every host.

Findings

⚠️ Majors

• [src/Interpreters/executeQuery.cpp:1524] ASTMoveAccessEntityQuery is not classified before auto-fill. With replicated access storage plus a local storage, MOVE USER u TO local_directory can get ON CLUSTER injected, so each DDL worker runs moveAccessEntities; one worker can remove the shared replicated entity before the others resolve it, or only one host can get the local copy. Suggested fix: skip MOVE when replicated access storage is involved, or resolve source and destination storages and auto-fill only when both sides are safe to move independently on every host.
• [src/Interpreters/executeQuery.cpp:1529] [dismissed by author -- https://github.com/Supports automatically fill on cluster value #94138#discussion_r3479141991] The access-storage predicate still checks whether any replicated access storage exists, not whether the explicit target storage is replicated. CREATE USER u IN local_directory / DROP USER u FROM local_directory therefore stay local even though explicit ON CLUSTER is valid and needed to keep the local storage consistent on every host. Suggested fix: make the access predicate target-storage aware and align removeOnClusterClauseIfNeeded with the same rule.
• [src/Interpreters/executeQuery.cpp:1668] [dismissed by author -- https://github.com/Supports automatically fill on cluster value #94138#discussion_r3411074214] The Replicated-database table-level ALTER skip is still based only on the database engine. For operations that DatabaseReplicated::shouldReplicateQuery explicitly does not replicate, such as DROP PARTITION, DROP DETACHED PARTITION, FREEZE / UNFREEZE, and UNLOCK SNAPSHOT, supported ON CLUSTER operations can be silently kept local, especially for non-replicated tables inside a Replicated database. Suggested fix: make the skip operation-aware; keep unsupported ATTACH / FETCH local, but either route supported non-database-replicated alters or document and test the narrower contract.

Tests

• ⚠️ Add integration coverage with both replicated and local access storage for explicit-storage access DDL and MOVE so the storage-selection contract is proven.
• ⚠️ Add a focused Replicated-database case for supported, non-database-replicated table-level ALTERs, or an explicit test documenting that auto-fill intentionally leaves them local.

Final Verdict

Status: ⚠️ Request changes

Minimum required actions: fix the new MOVE access-entity gap, and either fix or explicitly settle the two remaining semantic gaps around target-aware access storage and operation-aware Replicated-database ALTER handling.

• The behavior will conflict with the Replicated database.

Maybe we can use ignore_on_cluster_for_replicated_database added by #92872 to solve this? @alexey-milovidov

This needs more thought.

1. The behavior will conflict with the Replicated database.
2. The names are quite convoluted, we need something better, but at the same time, consistent with cluster_for_parallel_replicas.

This settings has now been marked as experimental, and a Context flag has been added to maybeRemoveOnCluster to resolve the conflict with ReplicatedDatabase. Could you please review it again? @alexey-milovidov

Picked this up to address the latest AI review round (the three new threads on executeQuery.cpp). Pushed 8f31df6fbc8..b46df0fb90a.

Addressed two of the three blockers (resolved their threads):

• Replicated access entities / named collections (b46df0fb90a). Access-control queries (CREATE USER/ROLE/QUOTA/ROW POLICY/SETTINGS PROFILE, DROP of access entities, GRANT) and named-collection queries (CREATE/ALTER/DROP NAMED COLLECTION) now feed the existing per-storage target_forbids_on_cluster decision, using the same predicates as removeOnClusterClauseIfNeeded: getAccessControl().containsStorage(ReplicatedAccessStorage::STORAGE_TYPE) and NamedCollectionFactory::instance().usesReplicatedStorage(). This stops auto-fill from turning a working local CREATE USER u (replicated access storage) into a CREATE USER u ON CLUSTER ... that the interpreter rejects. Non-replicated storage keeps ON CLUSTER eligible, exactly like UDFs/workloads/resources.

• Mixed multi-element RENAME (8f31df6fbc8). The RENAME branch now classifies every element and rejects a genuinely mixed statement (a Replicated-database source/destination alongside an ordinary target) with BAD_ARGUMENTS, mirroring the multi-table DROP guard you already added. The rejection happens during auto-fill, before InterpreterRenameQuery runs, so no ordinary target is renamed locally. Added Test 9 to 03789_automatic_fill_on_cluster_replicated_database proving the ordinary table is untouched after the rejected mixed RENAME.

Left the third blocker open for your call (Replicated-database skip for non-database-replicated table ALTERs — DROP PARTITION/FREEZE/etc.). I do not think this is a mechanical fix: DatabaseReplicated::shouldReplicateQuery returns false for those precisely because a ReplicatedMergeTree table replicates them through its own log, so keeping them local is not a silent loss — they still reach the other replicas. Routing them ON CLUSTER would push them to every node of the configured cluster in addition to the table’s own replication, which can double-apply when the cluster topology does not match the database’s replica set. There is also a practical obstacle: shouldReplicateQuery resolves the storage via getTable (throws on IF EXISTS/missing targets) and depends on runtime state not necessarily settled at the auto-fill point. So whether the conservative current behavior is correct is a design decision; details in the thread reply.

Test gap (the AI’s third ⚠️ test request). The replicated access / named-collection regression test needs server config (Keeper-backed user_directories/named_collections_storage), so it does not fit a plain stateless .sql; I left it out rather than ship a test I cannot run here. It is probably best as an integration test, or a stateless test with a dedicated config. Happy to add it if you prefer a particular shape.

CI. The pre-existing reds on 749680d3 are unrelated to this change: 03580_join_runtime_filter and 02973_block_number_sparse_serialization_and_mutation failed as Timeout in the slow amd_msan, WasmEdge shard, and the Performance Comparison failures (group_by_fixed_keys, count_from_formats) are perf-comparison instability — none can be affected by a default-off experimental setting that only touches the DDL auto-fill path. A fresh run is in progress on b46df0fb90a.

Build note. I could not run a full build in this environment (disk full), so the two C++ changes were verified with clang -fsyntax-only against the build’s compile command (clean), and Test 9’s rejection is deterministic ({ serverError BAD_ARGUMENTS }). The runtime paths would benefit from a local build + test run on your side before merge. No approval added — not my PR.

Picked this up to address the AI review (Request changes) about query-level SETTINGS ordering. Pushed b46df0fb90a..ed40d066300.

Setting-ordering fix (a9c63f9). The automatic ON CLUSTER fill decision in executeQueryImpl read allow_experimental_automatic_fill_on_cluster_mode and cluster_for_automatic_fill_mode before InterpreterSetQuery::applySettingsFromQuery had extracted the query-level SETTINGS from the AST, so the feature could not be switched per query. Moved applySettingsFromQuery up to right after the transaction check (and removed it from its later position so it still runs exactly once), so the auto-fill decision — and the corresponding interpreter — observe the per-query values. applySettingsFromQuery only ran after the decision before, and the settings reference points at the live context settings it mutates in place, so the values are now in effect for the block below.

New test 03789_automatic_fill_on_cluster_query_settings. Covers both required cases: profile off + SETTINGS allow_experimental_automatic_fill_on_cluster_mode = 1, cluster_for_automatic_fill_mode = 'test_shard_localhost' injects ON CLUSTER; profile on + SETTINGS allow_experimental_automatic_fill_on_cluster_mode = 0 stays local.

Verification against a locally built server (embedded Keeper + test_shard_localhost, distributed DDL working):

• A/B confirmed both directions (CREATE TABLE ... ON CLUSTER test_shard_localhost ... when enabled per query; plain local CREATE TABLE when disabled per query).
• New test passes 5/5 (not flaky); the two existing auto-fill tests still pass; a settings/log_queries regression batch — incl. 03290_mix_engine_and_query_settings (the engine/query settings separation this relies on), 01593_insert_settings, 01231_log_queries_min_type, 01546_log_queries_min_query_duration_ms — all pass.

Also merged latest master (the branch was ~1291 commits behind). The one remaining unresolved review thread is addressed and resolved.

Merged the current master (the branch was a week behind, ~2746 commits) to refresh CI on an up-to-date base. The merge was clean with no conflicts, and the PR's net diff vs master is unchanged (10 files, +664/-3).

One follow-up was needed: master opened a new 26.7 version block in SettingsChangesHistory.cpp while this PR's two settings were still registered under 26.6. Since 02995_new_settings_history requires settings absent from the 26.6.1 baseline to appear in a block greater than 26.6, I moved allow_experimental_automatic_fill_on_cluster_mode and cluster_for_automatic_fill_mode to the 26.7 block (commit fb448604f76).

Verified src/Interpreters/executeQuery.cpp and src/Core/SettingsChangesHistory.cpp still compile cleanly against the merged headers (no API drift). Fresh CI is running.

The two reds on the previous run were both confirmed master-wide flakes unrelated to this PR (off-by-default experimental feature): 03518_alter_logical_race timeout and the Cannot start clickhouse-server / Unexpected exception in refresh scheduling (STID 2508-34af) stress failure — both fail daily across many unrelated PRs on master.

After the master-merge earlier today, the AI review re-ran on the merged head and flipped to Request changes with two findings on executeQuery.cpp. Pushed fb448604f76..e6d709e6f9e.

Fixed: temporary-table guard for ALTER/OPTIMIZE (e6d709e6f9e). ALTER TABLE t and OPTIMIZE TABLE t with an omitted database resolve to a session-local temporary table even without the TEMPORARY keyword (the parser does not set isTemporary for them), so with auto-fill enabled the ASTQueryWithTableAndOutput branch filled a cluster and routed the operation through distributed DDL, leaving the session table untouched. The branch now applies the same resolves_to_temporary helper the DROP/TRUNCATE branch already used (hoisted to be shared): when the database is omitted and the unqualified name resolves via Context::ResolveExternal, the statement is treated as temporary and stays local. The check is guarded on a present table name so a database-level ALTER does not construct an empty StorageID. Regression added to 03789_automatic_fill_on_cluster_mode (Test 10): ALTER TABLE ... ADD COLUMN and OPTIMIZE TABLE on a session temporary MergeTree table, asserting the column is added to the session table and the statement carries no ON CLUSTER. Thread resolved.

Left open for your call: the access-storage predicate (executeQuery.cpp:1517). The bot wants auto-fill to resolve an explicit storage_name (e.g. CREATE USER u IN local_directory) and skip only when that storage is replicated, instead of skipping whenever any replicated access storage exists. The current predicate is intentionally byte-identical to the canonical removeOnClusterClauseIfNeeded — the single source of truth for which ON CLUSTER access queries are managed by replicated storage. Going finer-grained would diverge from it: for consistency removeOnClusterClauseIfNeeded would have to gain the same per-storage resolution, otherwise an explicit CREATE USER ... IN local_directory ON CLUSTER ... would still be stripped back to local (with ignore_on_cluster_for_replicated_access_entities_queries on) even though auto-fill added it. The failure mode is also conservative (the query stays local; the user can still add ON CLUSTER), unlike the temporary-table case which actively mis-routed a valid statement, and it cannot be covered by a plain stateless .sql (needs a server with both a replicated and a local access storage). So this is a behavior/design decision, not a mechanical parity fix — left the thread open for you / @JingYanchao.

Verification. The shared build directory here is in a divergent transitional state (a 4k-file rebuild), so I did not do a full local build; the change was verified with clang -fsyntax-only against the build's compile command (clean), the Test 10 SQL mechanics were checked against a running server (temporary MergeTree supports ALTER ADD COLUMN and OPTIMIZE, the default backfills), and the fix is a direct reuse of the already-CI-tested DROP/TRUNCATE temporary-table path (Test 8). The fresh CI run on e6d709e6f9e exercises Test 10 against the real PR binary.

CI on the prior head (fb448604f76) was green apart from two unrelated master-wide flakes: test_keeper_disks/test.py::test_snapshots_with_disks (assert 2 == 1 on snapshot file count) and Upgrade check (amd_release) (a MutatePlainMergeTreeTask CANNOT_PARSE_TEXT casting 'x' to UInt64 on a cached_s3 part) — neither can be affected by a default-off experimental setting that only touches the DDL auto-fill path.

No approval added — not my PR.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 115/127 (90.55%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 1 line(s) · Uncovered code

Full report · Diff report

Addressed the AI review Request changes finding on executeQuery.cpp. Pushed 0cd5eb3e862..65483540c47.

Fixed: persistent CREATE shadowed by a temporary table (65483540c47). The temporary-table resolution heuristic added for the implicit ALTER/OPTIMIZE/DROP case was applied to every ASTQueryWithTableAndOutput, including ASTCreateQuery. A persistent CREATE TABLE t / CREATE VIEW t creates a new object in the current database (a temporary and a persistent table of the same name can coexist), so it does not target a session temporary table just because one named t shadows it. With auto-fill enabled, the guard misclassified such a create as temporary, left cluster empty, and created the object only on the initiator instead of distributing an eligible CREATE ON CLUSTER. The fix excludes ASTCreateQuery from the heuristic; CREATE TEMPORARY TABLE is still kept local via the isTemporary check above, and the remaining statements that reach this branch (ALTER, OPTIMIZE, UPDATE, DELETE, CREATE/DROP INDEX) operate on an existing table by name, so the heuristic stays correct for them.

Added Test 11 to 03789_automatic_fill_on_cluster_mode: a persistent CREATE TABLE and CREATE VIEW shadowed by same-named temporary tables are rewritten ON CLUSTER, and the persistent objects are created.

Verification. -fsyntax-only of executeQuery.cpp against the real compile commands is clean. The temporary/persistent coexistence and DROP ordering used by the test were checked on a server, and the query_log LIKE patterns were confirmed to match the formatted ON CLUSTER queries (and not to self-match the verification SELECTs). The ON CLUSTER injection path itself is the same one already exercised by Test 2/Test 3; Test 11 validates it end-to-end in CI.

The remaining open thread (access-storage predicate at executeQuery.cpp:1529) is left as a design decision as discussed.