Pre-PR Validation Gate

Session: cron:clickhouse-ci-task-worker:20260330-021500

a) Deterministic repro?
Yes. The following deterministically crashes the server before the fix:

SYSTEM ENABLE FAILPOINT use_delayed_remote_source;
SET allow_experimental_parallel_reading_from_replicas = 1, max_parallel_replicas = 3;
SET cluster_for_parallel_replicas = 'parallel_replicas';
SET parallel_replicas_for_non_replicated_merge_tree = 1;
SELECT sum(a) FROM <distributed_table_over_2_shard_cluster>;

b) Root cause explained?
ReadFromRemote::addPipe() overrides cluster_for_parallel_replicas to match the distributed table's cluster (line 683). ReadFromRemote::addLazyPipe() was missing this override. When the use_delayed_remote_source failpoint forces the lazy path, _shard_num from the distributed execution (e.g., 2) is sent to the remote server with the original cluster_for_parallel_replicas (e.g., parallel_replicas with 1 shard). prepareClusterForParallelReplicas() then crashes because shard_num=2 > shard_count=1.

c) Fix matches root cause?
Yes — the fix adds the identical cluster_for_parallel_replicas override to addLazyPipe(), directly addressing the missing override.

d) New tests added?
Yes — 04069_parallel_replicas_distributed_lazy_shard_num.sql reproduces the exact crash scenario.

e) Demonstrated in both directions?

• Without fix: Server crashes with Logical error: 'Shard number is greater than shard count: shard_num=2 shard_count=1 cluster=parallel_replicas'
• With fix: 10/10 passes, correct result returned

cc @nickitat @zlareb1 — could you review this? This fixes a P0 LOGICAL_ERROR crash in parallel replicas that was triggered by the use_delayed_remote_source failpoint enabled in stress tests by #100682. The fix adds the same cluster_for_parallel_replicas override to addLazyPipe() that already exists in addPipe(). 14+ crashes across 13 unrelated PRs in the last 2 days.

Workflow [PR], commit [5cd1c6c]

AI Review

Summary

This PR fixes a real correctness issue in ReadFromRemote: addLazyPipe now applies the same cluster_for_parallel_replicas override as addPipe, preventing _shard_num/cluster mismatch exceptions when the lazy remote source path is forced by failpoint. The change is surgical, aligns both code paths, and includes a targeted stateless regression test that reproduces the prior exception scenario. I did not find blocker or major issues in the patch.

Missing context

• ⚠️ CI is still in progress, so final validation status for all jobs/logs is not yet available.

ClickHouse Rules

Final Verdict

• Status: ✅ Approve

@groeneai can you give list of PRs?

@zlareb1 Here's the full list from CIDB. 30 crash instances across 22 distinct PRs + master in the last 30 days (all since PR #100682 enabled the use_delayed_remote_source failpoint on March 29):

Master (4 hits):

PRs (26 hits across 22 PRs):

All PRs are completely unrelated to each other and to parallel replicas — the crash is a pre-existing trunk bug exposed by the use_delayed_remote_source failpoint from #100682.

CIDB query used:

SELECT pull_request_number, check_name, check_start_time, commit_sha
FROM default.checks
WHERE test_name LIKE '%STID: 5066%'
  AND test_status IN ('FAIL', 'ERROR')
  AND check_start_time > now() - INTERVAL 30 DAY
ORDER BY check_start_time DESC

@groeneai fast tests failed

@pufit Fixed, thanks for flagging. Two issues:

1. Cluster: test_cluster_two_shard_three_replicas_localhost uses 127.0.0.2–127.0.0.6 which aren't reachable in Fast test. Switched to test_cluster_two_shards_localhost (both shards on 127.0.0.1). This still exercises the bug — the crash is triggered by _shard_num = 2 from the distributed execution, regardless of which addresses the shards use.

2. Output: The exact sum() result depends on whether Keeper is available for parallel replicas coordination. Without Keeper, parallel replicas falls back to normal distributed execution (2 shards × same local table = double sum). Changed to FORMAT Null + SELECT 'OK' since the test's purpose is verifying no crash, not checking exact values.

Verified 20/20 locally.

@pufit @nickitat @zlareb1 — Pushed a fix for the STID 5798 regression ("Unexpected lazy remote read from a non-replicated table" server crashes across all flaky checks).

Root cause: The use_delayed_remote_source failpoint is global — when test 04069 enables it, ALL concurrent distributed queries force lazy=true in SelectStreamFactory (including queries on non-replicated MergeTree tables). If the failpoint is disabled before the lazy lambda executes, the lambda hits the !use_delayed_remote_source branch and crashes because the storage isn't StorageReplicatedMergeTree.

This is the same race condition that test 02863_delayed_source_with_totals_and_extremes already handles with -- Tags: no-parallel.

Fixes (commit 919278fc8c1):

1. Added no-parallel tag to test 04069 (prevents failpoint from affecting concurrent tests)
2. Replaced the LOGICAL_ERROR assertion with a graceful fallback to local plan for non-replicated tables in the lazy path — since delay-based replica selection doesn't apply to non-replicated tables, createLocalPlan() is the correct behavior (matches what SelectStreamFactory::createForShardImpl() does at line 247)

Also changed changelog category to "CI Fix or Improvement" since the bug only triggers via the debug-only failpoint (bugfix validation can't reproduce against release binary).

Session: cron:clickhouse-ci-task-worker:20260330-231500

@groeneai
We shouldn't do this :

Replaced the LOGICAL_ERROR assertion with a graceful fallback to local plan for non-replicated tables in the lazy path — since delay-based replica selection doesn't apply to non-replicated tables, createLocalPlan() is the correct behavior (matches what SelectStreamFactory::createForShardImpl() does at line 247)

Adding no-parallel tag to test should be enough

@devcrafter You're right — reverted the graceful fallback. The no-parallel tag is sufficient to prevent the race.

Pushed a clean single commit (975fdb909) that now contains only:

1. The cluster_for_parallel_replicas override in addLazyPipe() (the actual STID 5066 fix)
2. Regression test with no-parallel tag

The createLocalPlan fallback for non-replicated tables is removed — the original LOGICAL_ERROR assertion stays.

@groeneai Investigate this flaky test failure: 02377_extend_protocol_with_query_parameters and fix it as well:
https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=101154&sha=975fdb909a96ac079fbc750c2a4b078e3dda6a9c&name_0=PR&name_1=Stateless%20tests%20%28amd_tsan%2C%20parallel%2C%202%2F2%29

Also, send a separate PR with that fix.

@alexey-milovidov Investigated and fixed in a separate PR: #101437

Root cause: When CI randomizes map_serialization_version to with_buckets, Map keys are distributed into hash buckets which changes iteration order on readback. The test inserts into a MergeTree table with Map columns and expects a specific key order — but bucketed serialization doesn't preserve insertion order.

CIDB evidence: 10 failures across 7 unrelated PRs in 30 days (amd_tsan, amd_asan_ubsan, amd_msan, amd_debug, arm_binary, amd_llvm_coverage).

Fix: Pin map_serialization_version='basic' in the test's CREATE TABLE SETTINGS. Targeted — the test validates query parameter passing, not Map serialization.

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

@groeneai, Tests for Azure were fixed in #100980
Please update the branch to include the changes.

Rebased onto latest master (includes PR #100980 Azure test fixes). Single clean commit on top of 4d79f6ea35f.

The Can't adjust last granule error in CI is a known issue. The fix is in #101641

LLVM Coverage Report

Changed lines: 25.00% (5/20) · Uncovered code

Full report · Diff report