Workflow [PR], commit [bb002dd]

Dear @alexey-milovidov, this PR hasn't been updated for a while. Will you continue working on it? If not, please close it. Otherwise, ignore this message.

This is an actual bug, @alesapin should take a look at it.

Dear @alexey-milovidov, this PR hasn't been updated for a while. Will you continue working on it? If not, please close it. Otherwise, ignore this message.

This is very relevant - but waiting for @alesapin

Workflow [PR], commit [282f552]

Summary: ✅

AI Review

Summary

This PR adds a stateless shell test that stresses concurrent ALTER and INSERT on ReplicatedMergeTree and uses explicit retry/progress accounting so the empty-reference regression guard does not pass without exercising the intended race. I found no remaining review findings in the current diff; the earlier inline concerns are addressed or intentionally opted out in the current script, and the fetched Praktika report shows no failed jobs.

Final Verdict

Status: ✅ Approve

No. The original test didn't have IF EXISTS / IF NOT EXISTS. Having these breaks the purpose of the test.

Triage of the two failing checks on c082d7c7 (Fast test (arm_darwin)):

• 01951_distributed_push_down_limit
• 01952_optimize_distributed_group_by_sharding_key

Both are unrelated to this PR, which is test-only (it adds 03518_alter_logical_race.{sh,reference} and touches no distributed-query or planning code).

Evidence it is master-side, not this PR. Per CIDB, on 2026-06-14 each of these two tests failed with result differs with reference on 13 distinct PRs, exclusively on Fast test (arm_darwin), and on no day in the preceding two weeks — i.e. it started master-wide on 2026-06-14, not with this branch.

Root cause. Both tests run EXPLAIN ... remote('127.{1,2}', ...), i.e. a two-shard cluster on 127.0.0.1 + 127.0.0.2. macOS binds only 127.0.0.1 to lo0 and, unlike Linux, does not auto-route the rest of 127.0.0.0/8, so the 127.0.0.2 shard is classified differently than on Linux and the printed plan inlines the shard as a local read instead of ReadFromRemote:

-        ReadFromRemote
+        Sorting
+          Expression
+            ReadFromSystemNumbers

This is the same macOS loopback limitation behind the recent remote() test work:

• Related: Skip 02947_parallel_replicas_remote on macOS (arm_darwin) #107420 (merged 2026-06-14, skipped 02947_parallel_replicas_remote on macOS)
• Related: Add loopback interface aliases on macOS to run parallel-replica remote() tests #107435 (open — aliases 127.0.0.2..127.0.0.16 on lo0 and un-skips the parallel-replica remote() family)

Neither covers 01951/01952 (these are not in ci/defs/darwin.skip).

@groeneai, please confirm and handle these two as part of the macOS loopback remote() work (the lo0 alias / locality change in #107435 should make 127.0.0.2 reachable; if their plan still diverges on macOS, a reference adjustment or a darwin.skip entry would be needed), in a separate PR. Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=80416&sha=c082d7c7d2e4802cb9fee020ef5108323a5e84ad&name_0=PR&name_1=Fast%20test%20%28arm_darwin%29

I have refreshed the branch (merged current master) and pushed a fix for the one open review thread, so CI will re-run on d791063a20c.

@alexey-milovidov Confirmed. Both 01951_distributed_push_down_limit and 01952_optimize_distributed_group_by_sharding_key are the macOS-loopback EXPLAIN class and are unrelated to this test-only PR.

On the mechanism: aliasing 127.0.0.2..16 on lo0 makes those addresses both reachable and local. Reachable is what the connect-timeout family (02947 and the other parallel-replica remote() tests) needs, which is why #107435 un-skips them. But a lo0 alias is also a local address, so isLocalAddress() now matches 127.0.0.2 and the planner inlines that shard as a local read: EXPLAIN prints Sorting/Expression/ReadFromSystemNumbers instead of ReadFromRemote. That is exactly the divergence you noted, so darwin.skip is the right handling rather than a reference adjustment (an edited reference would still differ from Linux/master, where 127.0.0.2 is genuinely remote).

These two are already in ci/defs/darwin.skip on #107435's current HEAD (2e2061197e2), together with a teardown post_hook that de-aliases 127.0.0.2..16 at job end. The missing teardown was the real trigger for the 2026-06-14 fan-out: the macos_m2 runner is reused, so the pre_hook aliases leaked onto later jobs and made 127.0.0.2 look local to unrelated PRs.

On your separate-PR request: since the skip is needed precisely because of #107435's locality change, and both lines already sit on its darwin.skip diff, keeping them there avoids a darwin.skip conflict between two PRs and a window where #107435 merges (aliases live) before a standalone skip PR lands (failures would resume meanwhile). I am happy to split them into a dedicated darwin.skip PR and drop them from #107435 if you would rather. Which do you prefer?

Updated the test (commit a9670c9596b) and re-merged master (c8f85d17c6c..f955f70c22e, was 508 commits behind; clean, diff stays test-only).

Two changes:

1. Pinned randomized settings (no-random-settings, no-random-merge-tree-settings). The only PR-caused failure was 03518_alter_logical_race on Stateless tests (amd_msan, WasmEdge, parallel, 1/2), and CI's own randomized-settings diagnosis was conclusive: passes 3/3 without randomization, fails 3/3 with the randomized settings. The randomized merge-tree thresholds only slow the data-rewriting MODIFY COLUMN mutation enough to blow the per-worker time budget on a slow sanitizer build, so the loop completes no full cycle / lands no INSERT and exits non-zero. This is a timing-sensitive concurrency guard whose contract is orthogonal to query/merge-tree settings, so pinning them removes the variance without weakening the check.

2. Required INSERT progress (addresses the ⚠️ Request changes verdict / the line-185 thread). thread_insert now records landed INSERTs in .inserts.<id> files (separate from .cycles.<col>), and the gate fails with NO INSERT PROGRESS when zero INSERTs landed across both insert workers, with the same _disrupted shutdown waiver as the ALTER-cycle gate. A run where both insert workers only see retryable read-only/Keeper errors can no longer pass as a false green.

Triage of the remaining reds, all unrelated to this test-only PR:

• Stress test (arm_tsan) — Hung check failed, possible deadlock found: a master-wide flake (119 occurrences across 58 PRs including master in the last 5 days).
• Integration tests (amd_llvm_coverage, 1–5/5) and (old analyzer, … parallel): systemic amd_llvm_coverage job-level infra failures (red across many PRs while the test reports themselves show 0 test failures / empty results). A test-only PR adds no integration test.

The re-merge re-triggers CI on fresh master.

LLVM Coverage Report

Changed lines: No C/C++ source files changed — skipping uncovered code analysis.

Newly covered by added/modified tests: 387 line(s), 10 function(s) across 126 file(s) · Details

Full report