Conversation
Share cluster resolution via resolveClusterRead so getQueryProcessingStage matches read when object_storage_cluster_fallback_if_empty is enabled, and skip local object_storage_cluster lookup when object_storage_remote_initiator and object_storage_remote_initiator_cluster are both set. Co-authored-by: Cursor <cursoragent@cursor.com>
Cover pure fallback on unknown cluster, aggregate planning, remote-initiator interaction, and integration scenarios with locally unknown object_storage_cluster. Co-authored-by: Cursor <cursoragent@cursor.com>
Cover stateless and integration cases where object_storage_cluster is missing locally and remote initiator falls back to non-cluster execution. Co-authored-by: Cursor <cursoragent@cursor.com>
…uster functions. Distinguish s3() with object_storage_cluster setting from s3Cluster() argument so fallback works for the former but explicit cluster names still fail when unknown. Co-authored-by: Cursor <cursoragent@cursor.com>
Distinguish alternative-syntax table functions from table reads so remote initiator keeps s3() for fallback and uses cluster path for Iceberg tables. Co-authored-by: Cursor <cursoragent@cursor.com>
… cluster setting. Track explicit *Cluster function arguments separately from table engine and query object_storage_cluster settings so fallback works for persistent tables. Co-authored-by: Cursor <cursoragent@cursor.com>
Split storage policy from the setting check and route all local-fallback decisions through one helper so the resolve/read path is easier to follow. Co-authored-by: Cursor <cursoragent@cursor.com>
… syntax. Pure-send to a remote initiator requires object_storage_remote_initiator_cluster; otherwise keep the clustered path that defaults the initiator cluster to object_storage_cluster. Co-authored-by: Cursor <cursoragent@cursor.com>
…ster. Under remote-initiator deferral, an empty local cluster name must fall back to pure send so ENGINE=S3/Iceberg without object_storage_cluster does not hit LOGICAL_ERROR. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the redundant pure-send gate, flatten fallback_to_pure, and reuse the already resolved remote-initiator cluster on the clustered path. Co-authored-by: Cursor <cursoragent@cursor.com>
Local fallback applies only to object_storage_cluster; a bad remote-initiator cluster must still report CLUSTER_DOESNT_EXIST. Co-authored-by: Cursor <cursoragent@cursor.com>
Empty OSC with remote_initiator and no remote_initiator_cluster must keep BAD_ARGUMENTS even when fallback is enabled; extend tests for cases 1-3 and this regression. Co-authored-by: Cursor <cursoragent@cursor.com>
Pure-send ENGINE tables under remote-initiator deferral (preserving OSC in SETTINGS/context) so fallback matches s3() alternative syntax; extend TF and ENGINE tests for cases 1-3. Co-authored-by: Cursor <cursoragent@cursor.com>
Merge the two policy hooks into usesObjectStorageClusterSettingSyntax, share local-vs-remote fallback decisions between read and getQueryProcessingStage, and dedupe remote-initiator send. Co-authored-by: Cursor <cursoragent@cursor.com>
…TTINGS. object_storage_cluster may be unknown locally and defined on the remote (or the reverse); *Cluster would bake the name into the function argument and skip remote fallback. Co-authored-by: Cursor <cursoragent@cursor.com>
…cluster. Align flag semantics with s3()/iceberg() alternative syntax when the cluster name comes from SETTINGS rather than a *Cluster argument. Co-authored-by: Cursor <cursoragent@cursor.com>
…ter. AST rewrite and setting-syntax checks only need cluster_name_from_function_argument. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Failed stateless tests is flaky. |
…cluster_allow_empty
…cluster_allow_empty
arthurpassos
left a comment
There was a problem hiding this comment.
to be fairly honest, I couldn't do an useful review on this one in a timely manner. Too many branching in code I am not super familiar with. I'll just delay you more if I try to understand every bit of it.
AI review below:
Summary
The change is a gated fail-open for reads when object_storage_cluster names a missing or 0-node cluster. Writes and *Cluster stay fail-closed. That contract is internally consistent.
resolveClusterRead + the RI “send plain s3() + setting” path is the right design for “OSC exists only on the remote.” Tests lock the main s3() / S3 ENGINE / RI cases.
Findings
TableFunctionObjectStorage.cpp builds StorageObjectStorageCluster from cluster_for_parallel_replicas when OSC is unset. The PR defaults cluster_name_from_function_argument = false, so usesObjectStorageClusterSettingSyntax() is true.
Then getClusterName falls through to that constructor name. If fallback is on and the parallel-replicas cluster is unknown/empty, getClusterImpl(..., allow_null=true) turns a failed PR plan into a silent local s3() read.
That is realistic: the people who will put fallback in a profile are the same people who already set cluster_for_parallel_replicas.
Fix: do not apply OSC fallback unless the name actually came from object_storage_cluster (query/table/database). Parallel-replicas construction should look like a function-argument cluster (setClusterNameFromFunctionArgument(true) or a third origin).
The PR story is “no swarm node is alive.” The code checks tryGetCluster and getAllNodeCount() == 0. getAllNodeCount is configured replicas (per_replica_pools.size()), not live ones.
A named swarm with every host dead still has count > 0 → no fallback → ALL_CONNECTION_TRIES_FAILED / skip_unavailable_shards. Tests only use unknown names.
Either document that, or the stated swarm case is not solved.
read() / getQueryProcessingStage use resolveClusterRead. Public getCluster() still uses the constructor name and always throws. INSERT ... SELECT with parallel_distributed_insert_select can throw while the matching SELECT falls back. Fail-closed is defensible; the dual API is not.
💡 PR text uses the wrong setting name
Body: object_storage_cluster_fallback_if_empty. Code/tests: object_storage_cluster_fallback_to_local_if_empty. No alias.
Tests
Good for unknown-name s3() / ENGINE / RI / “don’t mask a missing RI-cluster.” Missing: write still fails, 0-node configured cluster, iceberg/DataLake, and the parallel-replicas collision above.
Verdict
Request changes if this is meant to be a profile default for swarms.
Minimum:
Exclude cluster_for_parallel_replicas from fallback.
Fix the changelog/setting name in the PR body.
Say “unknown or zero configured nodes,” not “swarm is down,” unless you add a liveness check (I would not).
CI triageNo PR-caused failures. The only red product test is pre-existing:
Grype alpine OpenSSL CVEs and the cancelled OAuth / GrypeScanServer jobs are infrastructure. Swarms coverageNot sufficient for the new fallback. The suite never sets That matrix lives in I'm going to add some swarms tests. |
|
Tests against this PR added here to swarms suite: https://github.com/Altinity/clickhouse-regression/blob/main/swarms/tests/fallback_to_local_if_empty.py Tests are passing. LGTM |
Rebase of #2028
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Allow empty object storage cluster
Documentation entry for user-facing changes
With 'object_storage_cluster' setting query to s3,iceberg and some other sources are executed as cluster request.
But with swarm cluster, when initiator is not a cluster member, may be situation when no one swarm node is alive at the moment. In this case query is failed with
CLUSTER_DOESNT_EXISTerror.New setting
object_storage_cluster_fallback_if_emptyallow to execute read query on local node in this case.Write query is not executed on cluster right now, so attempt to write is still failed in this case to avoid situation when query is success when swarm is empty and failed when has some nodes alive.
PR is a little bit complex because:
s3(...)- can fall back ifobject_storage_clusteris empty (cluster does not have active nodes, not 'empty setting value')s3(...) SETTINGS object_storage_remote_initiator=1- failed on local node ifobject_storage_clusteris emptys3(...) SETTINGS object_storage_remote_initiator=1, object_storage_remote_initiator_cluster='...'- decision about falling back must be made on remote initiator, on local nodeobject_storage_clustercan be unknown.But behavior is not changed for
Clusterfunctions:s3Cluster(...)- can't fall backs3Cluster(...) SETTINGS object_storage_remote_initiator=1- must failed on remote initiator ifobject_storage_clusteris empty.CI/CD Options
Exclude tests:
Regression jobs to run: