@codex review

Workflow [PR], commit [8fedfb5]

@codex review

@codex review

Codex Review: Didn't find any major issues. What shall we delve into next?

@codex review

@codex review

@codex review

Codex Review: Didn't find any major issues. Keep it up!

Audit: PR #1608 — Antalya 26.1: Remote initiator improvements 2

Source: Altinity/ClickHouse#1608
Base: antalya-26.1
Reviewed revision: branch including commit fb624cbad07 (Fix, current tip at audit time) and related feature commits (e.g. e1286a97214, merge 6a036176795 with antalya-26.1).

User-facing intent: Allow object_storage_remote_initiator with object_storage_remote_initiator_cluster when object_storage_cluster is defined only on the remote node (initiator runs s3/iceberg/… without a local cluster name; query is rewritten to remote(...)).

Confirmed defects

Medium — Remote initiator ignored when isClusterSupported() is false

• Impact: For StorageObjectStorageCluster backed by a configuration with isClusterSupported() == false (e.g. DataLakeConfiguration with is_cluster_supported = false, such as local Paimon / local Delta Lake templates in DataLakeConfiguration.h), a query with object_storage_remote_initiator = 1 and a non-empty object_storage_remote_initiator_cluster no longer takes the convertToRemote path. The new early return calls readFallBackToPure on the initiator only, so execution is not delegated to the remote cluster. Previously, the remote-initiator branch lived inside if (!isClusterSupported() || cluster_name_from_settings.empty()), so it still ran when cluster support was disabled but the local cluster name was empty (which is always the case here because StorageObjectStorageCluster::getClusterName returns "" when !isClusterSupported()).
• Anchor: IStorageCluster::read — src/Storages/IStorageCluster.cpp (early if (!isClusterSupported()) { readFallBackToPure; return; } before the object_storage_remote_initiator block).
• Trigger: object_storage_remote_initiator enabled, object_storage_remote_initiator_cluster set, storage configuration reports isClusterSupported() == false, local object_storage_cluster empty (typical for these configs).
• Why defect: Control-flow regression versus the prior combined condition; remote-initiator settings are silently ignored instead of forwarding the query, which can change where the query runs (initiator vs chosen remote host).
• Fix direction (short): Reorder or nest checks so object_storage_remote_initiator is evaluated for the empty–local-cluster case even when isClusterSupported() is false, or explicitly reject with BAD_ARGUMENTS if that combination is unsupported (fail closed with a clear error instead of silent local fallback).
• Regression test direction (short): Integration test: table function / engine path with isClusterSupported() == false, remote initiator settings, assert query runs on remote (or document and assert explicit error if intentionally unsupported).

Positive change in the same commit (not a defect)

• convertToRemote: Query-level SETTINGS now remove both object_storage_remote_initiator and object_storage_remote_initiator_cluster when stripping the clause, and the copied context resets both via resetSettingsToDefaultValue. This avoids leaving object_storage_remote_initiator_cluster in the AST while the initiator flag was already cleared.

Previously identified issue (resolved, earlier in PR)

With empty local object_storage_cluster, cluster_name_in_settings stayed false unless setClusterNameInSettings(true) ran for the remote-only Fallback path, which broke extractDynamicStorageType / named-collection handling for data-lake table functions.

• Resolution: TableFunctionObjectStorageClusterFallback::executeImpl calls setClusterNameInSettings(true) for every StorageObjectStorageCluster produced via BaseCluster::executeImpl when is_cluster_function || is_remote.

Coverage summary

Notable code anchors

• Early cluster-support exit: src/Storages/IStorageCluster.cpp — read() lines ~319–323 (!isClusterSupported() → readFallBackToPure).
• Remote initiator (cluster-capable, empty local cluster): same file — cluster_name_from_settings.empty() branch with object_storage_remote_initiator.
• Fallback + cluster_name_in_settings: src/TableFunctions/TableFunctionObjectStorageClusterFallback.cpp — executeImpl.
• getClusterName when cluster unsupported: src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp — returns "" if !isClusterSupported().

Audit metadata

• Method: Static review (call graph / control-flow diff vs previous antalya-26.1...HEAD, fault categories, spot-check for C++ / concurrency on touched paths).
• Output rule: Confirmed defects only in the main list; improvements called out separately.

AI-generated audit note.

Medium — Remote initiator ignored when isClusterSupported() is false

It is expected.

Usability Audit: Antalya 26.1 Remote Initiator Improvements 2 (PR #1608)

Scope: Altinity/ClickHouse#1608 @ fb624cb — allow object_storage_remote_initiator=1 + object_storage_remote_initiator_cluster='X' on initiator when object_storage_cluster is empty locally; query rewritten to remote('random_host_of_X', s3/iceberg(...)).
Persona: Operator / DBA (running cross-cluster object-storage queries from a front-end node).
Environment: Initiator node without a configured object_storage_cluster; remote cluster (object_storage_remote_initiator_cluster) possibly secured or behind a user profile.

Already-reported issues excluded from this report: Codex P1 (cluster-mode gate), Codex P2 (remote-initiator-cluster alone flipping cluster mode), Codex P2 (non-cluster-supported storages taking the rewrite path), Codex P2 (remote-initiator-cluster not stripped from forwarded AST), and the author-acknowledged "remote initiator ignored when isClusterSupported()==false" finding (author marked as expected).

Primary flows (ranked by expected usage)

1. SELECT ... FROM s3(...) / iceberg(...) SETTINGS object_storage_remote_initiator=1, object_storage_remote_initiator_cluster='swarm_cluster' from an initiator node with no local object_storage_cluster. (most common — the feature's advertised path per PR description and test_object_storage_remote_initiator_without_cluster_function)
2. Same call pattern but both object_storage_remote_initiator* settings come from a user profile (see osc profile + biz user in tests/integration/test_s3_cluster/configs/users.xml).
3. CREATE TABLE t AS s3(...) SETTINGS object_storage_remote_initiator=1, object_storage_remote_initiator_cluster='swarm_cluster' (natural user extension from the table-function path).

Issues

Flow 1: Ad-hoc SELECT with session/query settings

1.1 Misleading error when object_storage_remote_initiator=1 is set without any cluster setting

• Symptom: Setting 'object_storage_remote_initiator' can be used only with 'object_storage_remote_initiator_cluster' or 'object_storage_cluster' (src/Storages/IStorageCluster.cpp:341).
• Trigger: User enables object_storage_remote_initiator=1 but forgets both cluster settings. The error suggests object_storage_cluster as an alternative, but this branch is entered only when cluster_name_from_settings.empty() is already true — so if the operator follows the hint and sets object_storage_cluster, they will instead take a different (cluster) code path with different semantics, not fix the current error.
• Signal quality: Confusing.
• Severity: Medium.
• Recovery: Self-service once the operator reads the source; no docs yet.

1.2 Operator cluster misconfigurations surface as LOGICAL_ERROR

• Symptom: Empty cluster X or Size of shard N in cluster X is not equal 1 (src/Storages/IStorageCluster.cpp:443-444, :450-451).
• Trigger: object_storage_remote_initiator_cluster points at a cluster definition that has no shards, or any shard with more than one replica (after getClusterImpl flattening — e.g., if upstream flattening logic changes). The first is a plausible typo / stale config; the second is a latent assumption.
• Signal quality: Confusing — LOGICAL_ERROR tells the operator "ClickHouse bug", not "bad cluster config".
• Severity: Medium.
• Recovery: Runbook (operator must inspect cluster XML).

1.3 Clusters with <secret> are silently unsupported

• Symptom: Can't convert query to remote when cluster uses secret (NOT_IMPLEMENTED) at query time (src/Storages/IStorageCluster.cpp:439-440).
• Trigger: object_storage_remote_initiator_cluster names a cluster with a <secret> element — a standard production pattern for authenticated inter-node RPC.
• Signal quality: Silent until runtime; no mention in the setting description or PR docs; setting docs only say "use this cluster name".
• Severity: High — rules out all secret-secured clusters without warning.
• Recovery: Engineer required (change cluster config or route around the feature).

1.4 Random remote-initiator pick has no health check and no failover

• Symptom: rng() % host_addresses.size() picks one shard, then getMany is later driven with that single-host shard info; if the chosen host is offline, the entire SELECT fails even when other hosts in the same cluster are healthy (src/Storages/IStorageCluster.cpp:446-458).
• Trigger: One host in the remote cluster is down, restarting, or under admission pressure.
• Signal quality: Confusing — operator sees a generic connection error on the initiator and has no indication that a retry against a different host would succeed.
• Severity: High (for production: multi-node clusters exist specifically for HA).
• Recovery: Operator must retry the query manually, sometimes repeatedly, to hit a live host.

1.5 Chosen remote-initiator host is only visible in server INFO log

• Symptom: LOG_INFO(log, "Choose remote initiator '{}'", host_name) (src/Storages/IStorageCluster.cpp:455). The choice is not attached to system.query_log, system.processes, or the client response.
• Trigger: Operator investigating uneven per-query latency, per-host caching effects, or "which node did this query actually run on?".
• Signal quality: Silent at the client; confusing at scale.
• Severity: Medium.
• Recovery: Runbook (tail initiator server log, grep by query id — and ids correlate only loosely with log lines).

1.6 Every remote-initiator SELECT adds a DESCRIBE round-trip

• Symptom: Test asserts queries == ["3"] for one SELECT (initial + describe table + remote) in tests/integration/test_s3_cluster/test.py:548-550; the 5-count case shows initial + describe + remote + 2 subqueries.
• Trigger: Every remote-initiator SELECT — the storage resolution on the initiator performs a describe before the query is forwarded.
• Signal quality: Silent (no warning in docs about the cost).
• Severity: Low–Medium (noticeable at high QPS / short-query workloads).
• Recovery: Self-service (avoid the feature for short queries).

Flow 2: Settings from a user profile

2.1 Profile-carried object_storage_remote_initiator_cluster can flip plain table-function semantics without a local object_storage_remote_initiator flag

• Symptom: In parseArgumentsImpl, is_remote = settings[Setting::object_storage_remote_initiator]; executeImpl takes is_cluster_function || is_remote (src/TableFunctions/TableFunctionObjectStorageClusterFallback.cpp:120, :142). A profile setting object_storage_remote_initiator=1 applied globally changes every plain s3(...) / iceberg(...) in that session into cluster-mode storage construction, even when the caller only wanted the simple path.
• Trigger: Operator adds object_storage_remote_initiator=1 to a profile (the natural way to enable the feature for a whole team) and then any session that inherits the profile has its plain s3(...) calls silently rewritten.
• Signal quality: Silent.
• Severity: Medium.
• Recovery: Runbook (use query-level settings instead of profile; or set object_storage_remote_initiator=0 at session start).

Flow 3: CREATE TABLE t AS <table_function>

3.1 CREATE TABLE t AS s3(...) SETTINGS object_storage_remote_initiator=1, ... is accepted but produces a half-broken table

• Symptom: validateUseToCreateTable() only rejects is_cluster_function, not is_remote (src/TableFunctions/TableFunctionObjectStorageClusterFallback.cpp:154-161). So a CREATE TABLE t AS s3(...) SETTINGS object_storage_remote_initiator=1, object_storage_remote_initiator_cluster='X' succeeds and stores a StorageObjectStorageCluster with setClusterNameInSettings(true), but the remote-initiator settings themselves are not persisted into the table metadata. Later SELECT * FROM t without those session settings takes the pure-fallback branch at IStorageCluster.cpp:329-354 and runs locally — the table silently loses the feature the user created it with.
• Trigger: Operator treats CREATE TABLE AS ... as the stable form of an ad-hoc table function, as the docs for s3/iceberg encourage.
• Signal quality: Silent — no error at CREATE, no warning at SELECT, wrong routing.
• Severity: High.
• Recovery: Engineer required (DROP + recreate with the cluster in object_storage_cluster instead, or apply the settings as a user/profile default).

Cross-flow issues

X.1 Unexpected query AST shapes produce LOGICAL_ERROR

• Symptom: Expected SELECT query, Can't find table expression, Can't find table function in table expression (src/Storages/IStorageCluster.cpp:466-467, :481-484).
• Trigger: Any user query shape in which query_info.query is not a plain ASTSelectQuery with a table-function table expression — e.g., rewrites from views, CTEs, JOIN pushdowns with an ASTTableIdentifier on the storage side, or queries restructured by analyzer passes in future changes. Reachable by ordinary SQL, not just server bugs.
• Signal quality: Confusing — LOGICAL_ERROR suggests a server bug.
• Severity: Medium.
• Recovery: Engineer required.

X.2 No system.* surface announces the feature took effect

• Symptom: There is no system.settings-level explanation, no system.object_storage_queries-style table, and no per-query marker that says "this query was forwarded to remote host H via remote-initiator". Operators validating a rollout must rely on cross-referencing system.query_log (initial_query_id) with initiator server logs.
• Trigger: Rollout verification, SRE audit, capacity planning.
• Signal quality: Silent.
• Severity: Low–Medium.
• Recovery: Runbook (cross-join clusterAllReplicas(..., system.query_log) on initial_query_id, as the new integration test does).

Readiness verdict

• Flow 1 (ad-hoc SELECT with remote initiator) in isolation: ready with caveats; happy path works (tests pass), but Update README.md #1.3 rules out secret-secured clusters, Update README.md #1.4 removes expected failover, and Update README.md #1.2 / X.1 mis-classify operator errors as server bugs.
• Flow 2 (profile-carried settings) in isolation: not ready; Update README.md #2.1 makes profile-level enablement dangerous for any session that also does plain table-function calls.
• Flow 3 (CREATE TABLE t AS ...) in isolation: not ready; Update README.md #3.1 silently produces a table that loses the feature.
• Overall in production (network blips, concurrent admin ops, scale): ready with caveats (guardrails required), because of Update README.md #1.3 (secret clusters unsupported), Update README.md #1.4 (no failover), Update README.md #2.1 (profile semantics), Update README.md #3.1 (broken CREATE TABLE persistence), with secondary friction from Update README.md #1.1, Update README.md #1.2, Update README.md #1.5, Update README.md #1.6, X.1, X.2.

Operator guidance (until fixes land)

• Do not enable object_storage_remote_initiator / object_storage_remote_initiator_cluster via a profile. Set them per-query or per-session only (Update README.md #2.1).
• Do not use CREATE TABLE t AS s3(...) SETTINGS object_storage_remote_initiator=.... Keep remote-initiator usage in the query layer, not in persistent DDL (Update README.md #3.1).
• Before rolling the feature out, verify the target object_storage_remote_initiator_cluster has no <secret> element (Update README.md #1.3) and that each shard in it is flattened to single-replica form (Update README.md #1.2).
• Assume no failover: run a smoke probe against every replica of the remote cluster and alert if any is down; expect tail failures until all are healthy (Update README.md #1.4).
• For operator debugging, add a grep runbook that joins system.query_log.initial_query_id with initiator LOG_INFOs Choose remote initiator '<host>' to establish host routing (Update README.md #1.5, X.2).
• Budget a ~1 extra metadata round-trip per query when using this feature; avoid it for sub-100ms queries (Update README.md #1.6).

Recommended fixes

• Reword the BAD_ARGUMENTS message at IStorageCluster.cpp:341 to only mention object_storage_remote_initiator_cluster for this branch (Update README.md #1.1).
• Reclassify Empty cluster X and Size of shard N ... is not equal 1 to BAD_ARGUMENTS (or CLUSTER_DOESNT_EXIST) and include the setting name the operator supplied (Update README.md #1.2).
• Validate <secret>-presence up front and either support it or fail fast with a clear BAD_ARGUMENTS; document the restriction on the setting (Update README.md #1.3).
• Build a multi-host connection pool and retry the rewritten remote(...) call against a different host on connect failure (Update README.md #1.4).
• Add a remote_initiator_host column (or equivalent ProfileEvents / query_log.used_remote_hosts entry) populated by convertToRemote so operators can see routing without log scraping (Update README.md #1.5, X.2).
• Reject is_remote in validateUseToCreateTable() the same way is_cluster_function is rejected, or persist the settings into the stored engine metadata so SELECT keeps the remote-initiator behavior (Update README.md #3.1).
• Gate is_remote = true in parseArgumentsImpl on the caller having explicitly set object_storage_remote_initiator at session or query scope, not at profile scope — or emit a warning in system.warnings when the profile-level setting activates (Update README.md #2.1).
• Convert the LOGICAL_ERROR guards in convertToRemote to BAD_ARGUMENTS with the offending AST stringified, so user-reachable shapes stop looking like server bugs (#X.1).

1.1 - known, expected.
1.2 - empty cluster is an user error, cluster with more than one replica in shards is impossible here - cluster is extracted with getClusterWithReplicasAsShards methos, each replica is converted in separate shard.
1.3 - known. Initiator is created with remote function, this function can't work with secrets now.
1.4 - possible improvement
1.5 - yes, that's true. I can't find other place except logs where to put this information. But for example cluster function chooses random replica for each shard, so this 'silent' behavior is not unique.
1.6 - known.
2.1 - known. global setting object_cluster_remote_initiator is a bad idea, but yes, user can shoot in own leg using this way.
3.1 - good catch, need to fix