All committers have signed the CLA.

Could you please edit your commits to include your email from GitHub?

Workflow [PR], commit [7911ab3]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR disables enable_parallel_replicas on the copied MutationsInterpreter context, so mutation source reads keep using the normal storage read path instead of entering legacy parallel-replica query rewriting with a synthetic table-less SELECT. The regression test now pins both enable_analyzer = 0 and automatic_parallel_replicas_mode = 0, so it exercises the legacy path that caused the reported SEGV. I found no remaining blockers or major issues.

Final Verdict

✅ No blocking or major review findings. The earlier inline concern about automatic_parallel_replicas_mode is addressed in the current code.

Addressed @devcrafter's feedback and rebased onto current master.

Reworked the fix as suggested. Instead of plumbing a SelectQueryInfo::parallel_replicas_disabled flag through StorageMergeTree and StorageReplicatedMergeTree, parallel replicas are now disabled directly on the per-mutation context in the MutationsInterpreter constructor:

/// Mutation source reads build a synthetic `SELECT` without a table expression,
/// so parallel replicas must not be used for them.
new_context->setSetting("enable_parallel_replicas", Field(0));

This is the single chokepoint all MutationsInterpreter constructors delegate to, and it covers both the task-based and custom-key paths uniformly (both canUseParallelReplicasOnInitiator and canUseParallelReplicasCustomKey gate on enable_parallel_replicas). The storage read methods are now untouched. This also became necessary because the parallel_replicas_disabled field the previous approach relied on was removed from SelectQueryInfo in 274427fb0de in the meantime.

Verification (local non-replicated MergeTree, three-replica cluster):

• Without the fix, enable_analyzer = 0: reproduces the exact SEGV from LWU with parallel replicas SEGV #104197 (rewriteSelectQuery → DatabaseAndTableWithAlias on the table-less synthetic SELECT).
• With the fix: the UPDATE succeeds and the server stays alive.

I also added enable_analyzer = 0 to the regression test so it deterministically exercises the legacy path that crashes, rather than depending on randomized settings.

(@www-dev-1 — the request to set your GitHub email on the commits is still on the original commit; that one needs to be done from your side.)

Merged current master and addressed the remaining review thread by pinning automatic_parallel_replicas_mode = 0 in the regression test (7911ab39259), so it deterministically exercises the legacy parallel-replica path.

The sole CI red — AST fuzzer (amd_debug): Logical error: Column identifier A is already registered (STID: 4697-4326) — is unrelated to this change. The fuzzer crash is an analyzer bug in subquery set building (buildSubqueryPlansForSetsAndAdd → Planner → GlobalPlannerContext::createColumnIdentifier) triggered by fuzzed ALTER ... UPDATE ... if(in(t0__fuzz_21, (SELECT DISTINCT ...))) queries. It has nothing to do with parallel replicas — this PR's only source change disables enable_parallel_replicas in the mutation context. It is tracked by the open issue #106649 (and historically by the closed #99931, exact same STID).

@groeneai, please investigate the failure https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=104255&sha=a171461d9fa04f80eee52bf67ca38389bebd3e16&name_0=PR&name_1=AST%20fuzzer%20%28amd_debug%29 and provide a fix in a separate PR. If a fix for #106649 is already in progress, link it here.

Linking the in-flight fix: PR #106025 addresses STID 4697-4326 / Issue #106649 (Logical error: Column identifier ... is already registered). It is a chronic trunk bug in the analyzer's subquery set planning (buildSubqueryPlansForSetsAndAdd -> Planner -> GlobalPlannerContext::createColumnIdentifier), unrelated to the parallel-replicas mutation context disabled in this PR. PR #106025 is currently OPEN with CHANGES_REQUESTED; PR #106414 (sibling, parallel attempt for the same root cause) is also in flight. Neither failure here is caused by this PR's changes.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 5/5 (100.00%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

@groeneai could you create new PR with the same fix? I can't simply reopen PR since @LambertArm has deleted his repository.

@devcrafter I recreated this fix as a new PR from our fork (since the original repo was deleted and this couldn't be reopened): #107246

It carries the exact approved final diff (the MutationsInterpreter-context approach + the #104197 regression test pinning enable_analyzer=0 and automatic_parallel_replicas_mode=0), Critical Bug Fix category, Closes #104197. Verified both directions locally: without the fix the UPDATE SEGVs at ClusterProxy::rewriteSelectQuery, with the fix it succeeds and the server stays alive.