Fix missing materialized-CTE gate when a Merge table has several children - #113558
Fix missing materialized-CTE gate when a Merge table has several children#113558alexey-milovidov wants to merge 1 commit into
Merge table has several children#113558Conversation
…ildren
`ReadFromMerge::createChildrenPlans` optimizes every child plan on its own, and
`resolveMaterializingCTEs` claims a materialized CTE globally through
`MaterializedCTE::is_materialization_planned`. The first child plan to be
optimized therefore moved the CTE's plan into *its own* tree, and every other
`DelayedMaterializingCTEsStep` for that CTE - in the sibling children and in the
outer plan - degenerated into a gate-less `MaterializingCTEsStep`. The CTE's
writer then sat in one child's pipeline while readers sat in another, with no
`DelayedPortsProcessor` between them, so a sibling's in-place `IN`-set build read
the storage while it was still empty and `ReadFromMemoryStorageStep` raised
`Reading from materialized CTE '...' before its materialization completed -
DelayedPortsProcessor gate is missing in the query plan`.
Reproducer on master (deterministic, also with `max_threads = 1`, and a plain
`EXPLAIN` is enough because the set is built during plan optimization):
SET enable_analyzer = 1, enable_materialized_cte = 1;
CREATE TABLE t (x UInt64) ENGINE = MergeTree ORDER BY x;
CREATE TABLE tdist AS t ENGINE = Distributed(test_shard_localhost, currentDatabase(), t);
CREATE VIEW tconst AS SELECT toUInt64(1) AS x;
WITH t AS MATERIALIZED (SELECT number AS c FROM numbers(2))
SELECT count() FROM merge(currentDatabase(), '^(tconst|tdist)$')
WHERE (x IN (t)) AND (x NOT IN (t));
Children are visited in table-name order, so `tconst` is planned first and claims
the CTE; the `Distributed` child that follows then builds the set in place while
the CTE is unbuilt. Renaming so the `Distributed` child sorts first makes the same
query pass, which is what pins the mechanism.
Fix: a child plan no longer claims a CTE that the outer query references as well.
`removeDelayedMaterializingCTEsStepFor` strips those steps from the child plan
before it is optimized, leaving the outer plan - whose `MaterializingCTEsStep`
sits above the whole merge - as the single owner that gates every child. This is
the same reasoning `DelayedCreatingSetsStep::makePlansForSets` already applies to
pre-built `IN`-subquery plans. A CTE defined inside one child (a `View` with its
own `WITH ... AS MATERIALIZED`) is not referenced by the outer query, so that
child keeps owning it - it is the only reader.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Workflow [PR], commit [f69033e] Summary: ❌
AI ReviewSummaryThis PR fixes the Final Verdict✅ No findings. The LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 40/47 (85.11%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
|
🕵 The only red check, AST fuzzer (amd_debug, targeted, old_compatibility) — Fix in progress: #113224 |
| static void removeDelayedMaterializingCTEsStepIf( | ||
| QueryPlan & plan, const std::function<bool(DelayedMaterializingCTEsStep &)> & should_splice_out) |
There was a problem hiding this comment.
| static void removeDelayedMaterializingCTEsStepIf( | |
| QueryPlan & plan, const std::function<bool(DelayedMaterializingCTEsStep &)> & should_splice_out) | |
| static void removeDelayedMaterializingCTEsStepIf( | |
| QueryPlan & plan, | |
| const std::function<bool(DelayedMaterializingCTEsStep &)> & predicate) |
2 participants
Related: #113184
Related: #113489
Related: #111194
Related: #113043
Related: #108924
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed the
LOGICAL_ERRORReading from materialized CTE '...' before its materialization completed - DelayedPortsProcessor gate is missing in the query planraised when aMergetable with more than one child reads a materialized CTE that the outer query references.Description
ReadFromMerge::createChildrenPlansoptimizes every child plan on its own, andresolveMaterializingCTEsclaims a materialized CTE globally throughMaterializedCTE::is_materialization_planned. The first child plan to be optimized therefore moved the CTE's plan into its own tree, and every otherDelayedMaterializingCTEsStepfor that CTE - in the sibling children and in the outer plan - degenerated into a gate-lessMaterializingCTEsStep. The CTE's writer then sat in one child's pipeline while readers sat in another, with noDelayedPortsProcessorbetween them, so a sibling's in-placeIN-set build read the storage while it was still empty andReadFromMemoryStorageStepraised the exception (in debug and sanitizer builds it aborts the server).Reproducer on master, deterministic (20/20), also with
max_threads = 1, and a plainEXPLAINis enough because the set is built during plan optimization:Children are visited in table-name order, so
tconstis planned first and claims the CTE, and theDistributedchild that follows builds the set in place while the CTE is unbuilt. Renaming so theDistributedchild sorts first makes the same query pass, which is what pins the mechanism.Fix. A child plan no longer claims a CTE that the outer query references as well.
removeDelayedMaterializingCTEsStepForstrips those steps from the child plan before it is optimized, leaving the outer plan - whoseMaterializingCTEsStepsits above the whole merge - as the single owner that gates every child. This is the same reasoningDelayedCreatingSetsStep::makePlansForSetsalready applies to pre-builtIN-subquery plans. The set of CTEs to strip comes from walking the outerquery_info.query_tree, so a CTE defined inside one child (aViewwith its ownWITH ... AS MATERIALIZED) is left owned by that child - it is the only reader, and stripping it unconditionally would leave it with no materialization at all.Validation. New
04811_materialized_cte_merge_child_gatecovers the failing child order, the explicit-subquery form, a satisfiable predicate that pins the data rather than only the absence of the exception, theEXPLAINroute, the reverse child order that always worked, and the view-owned-CTE case that must keep materializing inside the child. Every failing arm reproduces 5/5 on a master binary and passes 5/5 after the change. Thematerialized_ctesuite is green.This is one shape of a recurring family - the same assertion is also reported in #113184 and addressed for other shapes by #113489, #111194 and #113043 - so the underlying
is_materialization_plannedclaim being global while the gate is per-plan is worth revisiting separately. It surfaces constantly in the AST fuzzer; found viahttps://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=9d0b1a25ba7aa4579c95a65baca002d1dd7a1e47&name_0=MasterCI&name_1=AST%20fuzzer%20%28amd_debug%29