iframe-proxy

alexey-milovidov · 2026-03-13T06:33:40Z

Summary

Add max_skip_unavailable_shards_num (UInt64, default 0) setting: when skip_unavailable_shards is enabled, if the number of unavailable shards exceeds this value, throw TOO_MANY_UNAVAILABLE_SHARDS instead of silently skipping. A value of 0 means no limit.
Add max_skip_unavailable_shards_ratio (Float, default 0) setting: same but based on the ratio of unavailable shards to total shards. A value of 0 means no limit.
This prevents silently returning incomplete results when a large portion of the cluster is down.

Test plan

Added stateless test 04038_max_skip_unavailable_shards that verifies:
- Settings have no effect when skip_unavailable_shards is disabled
- Queries succeed when within the threshold (both num and ratio)
- TOO_MANY_UNAVAILABLE_SHARDS is thrown when thresholds are exceeded
- Both 2-shard and 3-shard configurations are tested

Changelog category:

New Feature

Changelog entry:

Added max_skip_unavailable_shards_num and max_skip_unavailable_shards_ratio settings to limit how many shards can be silently skipped when skip_unavailable_shards is enabled. If the number or ratio of unavailable shards exceeds the configured threshold, an exception is thrown instead of returning silently incomplete results.

🤖 Generated with Claude Code

Note

Medium Risk
Touches distributed query execution to optionally throw when too many shards are skipped, which could change runtime behavior for users enabling the new limits. Default settings preserve existing behavior but the new shared tracker/exception path needs careful review for concurrency and query fan-out cases.

Overview
Adds two new settings, max_skip_unavailable_shards_num and max_skip_unavailable_shards_ratio, to cap how many shards can be silently skipped when skip_unavailable_shards is enabled; exceeding either threshold now throws TOO_MANY_UNAVAILABLE_SHARDS instead of returning partial results.

Implements a shared UnavailableShardTracker wired from executeQuery into ReadFromRemote/RemoteQueryExecutor so each skipped shard is counted once across the whole distributed query, and adds a stateless test covering success/error cases for both numeric and ratio limits.

^{Written by Cursor Bugbot for commit 01d35a1. This will update automatically on new commits. Configure here.}

Version info

Merged into: 26.3.1.704

…s_ratio` settings When `skip_unavailable_shards` is enabled, these settings limit how many shards can be silently skipped before throwing an exception: - `max_skip_unavailable_shards_num` (UInt64, default 0): if the number of unavailable shards exceeds this value, throw `TOO_MANY_UNAVAILABLE_SHARDS`. A value of 0 means no limit. - `max_skip_unavailable_shards_ratio` (Float, default 0): if the ratio of unavailable shards to total shards exceeds this value, throw `TOO_MANY_UNAVAILABLE_SHARDS`. A value of 0 means no limit. This prevents silently returning incomplete results when a large portion of the cluster is down. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

clickhouse-gh · 2026-03-13T06:34:18Z

Workflow [PR], commit [4da7764]

Summary: ✅

AI Review

Summary

This PR adds max_skip_unavailable_shards_num and max_skip_unavailable_shards_ratio to limit silently skipped shards when skip_unavailable_shards is enabled. The overall direction is good, but there are two correctness gaps: ratio input is not range-validated, and ratio denominator currently uses full cluster shard count instead of queried shard count.

Missing context

⚠️ No CI logs/results were provided in this review task.

Findings

⚠️ Majors
- [src/Interpreters/ClusterProxy/executeQuery.cpp:464] UnavailableShardTracker is initialized with total_shards = cluster->getShardCount(), but query planning can target only a subset of shards (for example after sharding-key pruning). This makes max_skip_unavailable_shards_ratio too permissive and can miss expected exceptions.
  - Suggested fix: initialize tracker denominator from the actually queried shard set (for example remote_shards.size() after planning), not full cluster size.
- [src/Core/Settings.cpp:1236, src/Interpreters/ClusterProxy/executeQuery.cpp:462] max_skip_unavailable_shards_ratio is documented as [0, 1], but out-of-range values are accepted. Negative values silently disable ratio checks, and values >1 make threshold unreachable.
  - Suggested fix: validate on read/construction and throw BAD_ARGUMENTS for values outside [0, 1].

Tests

⚠️ Add a stateless test where sharding-key pruning reduces the queried shard set and one queried shard is unavailable; verify ratio threshold is computed over queried shards.
⚠️ Add validation tests for max_skip_unavailable_shards_ratio < 0 and > 1 to ensure BAD_ARGUMENTS is thrown.

ClickHouse Rules

Item	Status	Notes
Deletion logging	➖
Serialization versioning	➖
Core-area scrutiny	✅
No test removal	✅
Experimental gate	➖
No magic constants	✅
Backward compatibility	⚠️	Out-of-range ratio values currently change semantics silently instead of being rejected.
`SettingsHistory.cpp`	✅
Safe rollout	⚠️	Ratio behavior can be unexpectedly permissive on pruned queries due to denominator choice.
Compilation time	✅

Performance & Safety

Safety: threshold enforcement can be bypassed unintentionally (invalid ratio values and denominator mismatch), causing silently incomplete results where an exception is expected.

Final Verdict

Status: ⚠️ Request changes
Minimum required actions:
- Validate max_skip_unavailable_shards_ratio range to [0, 1].
- Compute ratio denominator from actually queried shards, not full cluster shard count.
- Add tests for both cases.

…s_ratio` to SettingsChangesHistory Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

orloffv · 2026-03-13T08:44:48Z

@alexey-milovidov
One request: when shards are skipped within the threshold (no exception thrown), could the response include some signal that the result is partial — e.g. an HTTP header like X-ClickHouse-Unavailable-Shards: 2/5, or a counter in the progress/metadata block?
Without this, callers can't distinguish a complete result from a silently degraded one, which makes the feature risky to use in production.

alexey-milovidov · 2026-03-13T19:56:06Z

The same UnavailableShardTracker is attached to multiple RemoteQueryExecutor instances that can correspond to one physical shard in the parallel-replicas custom-key path, so one unavailable shard can be counted multiple times and trigger TOO_MANY_UNAVAILABLE_SHARDS incorrectly.

This is alright.

…re in headers Avoid including `UnavailableShardTracker.h` (which pulls in `Common/Exception.h`) from high-fan-out headers `RemoteQueryExecutor.h` and `ReadFromRemote.h`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-03-13T21:28:28Z

@orloffv, yes, I want to add it. It will be one of the next PRs.

clickhouse-gh · 2026-03-16T07:34:48Z

LLVM Coverage Report

PR changed lines: PR changed-lines coverage: 99.16% (118/119, 0 noise lines excluded)
Diff coverage report
Uncovered code

clickhouse-gh Bot added the pr-feature Pull request with new product feature label Mar 13, 2026

clickhouse-gh Bot reviewed Mar 13, 2026

View reviewed changes

Comment thread src/Processors/QueryPlan/ReadFromRemote.cpp

Add max_skip_unavailable_shards_num and `max_skip_unavailable_shard…

01d35a1

…s_ratio` to SettingsChangesHistory Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

clickhouse-gh Bot reviewed Mar 13, 2026

View reviewed changes

Comment thread src/QueryPipeline/RemoteQueryExecutor.h

clickhouse-gh Bot reviewed Mar 13, 2026

View reviewed changes

Comment thread src/Interpreters/ClusterProxy/executeQuery.cpp

Merge master into max_skip_unavailable_shards

4da7764

clickhouse-gh Bot reviewed Mar 16, 2026

View reviewed changes

Comment thread src/Interpreters/ClusterProxy/executeQuery.cpp

alexey-milovidov self-assigned this Mar 16, 2026

alexey-milovidov added this pull request to the merge queue Mar 16, 2026

Merged via the queue into master with commit 5168c3b Mar 16, 2026
163 checks passed

alexey-milovidov deleted the max_skip_unavailable_shards branch March 16, 2026 12:29

robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 16, 2026

ianton-ru mentioned this pull request Jun 29, 2026

Antalya 26.5: 26.1 Antalya port - improvements for cluster requests Altinity/ClickHouse#1953

Open

27 tasks

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add max_skip_unavailable_shards_num and max_skip_unavailable_shards_ratio settings#99369

Add max_skip_unavailable_shards_num and max_skip_unavailable_shards_ratio settings#99369
alexey-milovidov merged 4 commits into
masterfrom
max_skip_unavailable_shards

alexey-milovidov commented Mar 13, 2026 •

edited by robot-ch-test-poll

Loading

Uh oh!

clickhouse-gh Bot commented Mar 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

orloffv commented Mar 13, 2026

Uh oh!

alexey-milovidov commented Mar 13, 2026

Uh oh!

alexey-milovidov commented Mar 13, 2026

Uh oh!

Uh oh!

Uh oh!

clickhouse-gh Bot commented Mar 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

alexey-milovidov commented Mar 13, 2026 • edited by robot-ch-test-poll Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Changelog category:

Changelog entry:

Version info

Uh oh!

clickhouse-gh Bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Missing context

Findings

Tests

ClickHouse Rules

Performance & Safety

Final Verdict

Uh oh!

Uh oh!

Uh oh!

orloffv commented Mar 13, 2026

Uh oh!

alexey-milovidov commented Mar 13, 2026

Uh oh!

alexey-milovidov commented Mar 13, 2026

Uh oh!

Uh oh!

Uh oh!

clickhouse-gh Bot commented Mar 16, 2026

LLVM Coverage Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexey-milovidov commented Mar 13, 2026 •

edited by robot-ch-test-poll

Loading

clickhouse-gh Bot commented Mar 13, 2026 •

edited

Loading