Fix missing materialized-CTE gate when a Merge table has several children
Project: ClickHouse/ClickHouse | Language: C++ | Concepts: query-optimization, data-structures, error-handling | Difficulty: senior
Link: https://github.com/ClickHouse/ClickHouse/pull/113558
The problem
When querying a Merge table with multiple children, if both the outer query and one or more children reference the same materialized CTE (Common Table Expression), the query plan can end up with missing synchronization ("gate") between the writer (materializing the CTE) and readers (children consuming the CTE). This leads to a LOGICAL_ERROR at runtime:
Reading from materialized CTE '...' before its materialization completed - DelayedPortsProcessor gate is missing in the query plan.
Why it's non-trivial
A naive fix might try to always materialize the CTE at the outermost level, or let each child handle its own CTE references. However, the query plan optimizer in ClickHouse allows each child plan to be optimized independently. If a child "claims" the CTE first, it can move the materialization plan into its own subtree, leaving sibling children and the outer query with degenerate (non-gated) references. This causes readers to access the CTE before it is fully built, leading to subtle, order-dependent failures that are hard to debug.
The approach
1. Collect all materialized CTEs referenced by the outer query:
+/// Every materialized CTE reachable from `node`. Pointer identity is what matters:
+/// all references to one CTE - including the ones a child plan resolves by name to
+/// the CTE's temporary `StorageMemory` - share the same `MaterializedCTE` object.
+static MaterializedCTESet collectMaterializedCTEsFromQueryTree(const QueryTreeNodePtr & node)
+{
+ MaterializedCTESet result;
+ if (!node)
+ return result;
+
+ traverseQueryTree(node, Everything{},
+ [&](const QueryTreeNodePtr & current_node)
+ {
+ if (const auto * table_node = current_node->as<TableNode>())
+ {
+ if (auto cte = table_node->getMaterializedCTE())
+ result.insert(std::move(cte));
+ }
+ });
+
+ return result;
+}
This function walks the query tree and collects all unique materialized CTEs referenced by the query, identified by pointer identity.
2. Remove CTE claims from child plans for CTEs also referenced by the outer query:
+void removeDelayedMaterializingCTEsStepFor(QueryPlan & plan, const MaterializedCTESet & ctes_to_remove)
+{
+ if (ctes_to_remove.empty())
+ return;
+
+ removeDelayedMaterializingCTEsStepIf(
+ plan, [&](DelayedMaterializingCTEsStep & step) { return step.eraseCTEs(ctes_to_remove); });
+}
This function strips ownership of the relevant CTEs from a child plan, ensuring only the outer plan owns their materialization and gating.
3. Integrate this logic into the child plan creation for Merge tables:
@@ -1141,6 +1182,8 @@ std::vector<ReadFromMerge::ChildPlan> ReadFromMerge::createChildrenPlans(SelectQueryInfo & query_info_) const
child.plan.addStep(std::move(filter_step));
}
+ removeDelayedMaterializingCTEsStepFor(child.plan, outer_materialized_ctes);
+
child.plan.optimize(getChildPlanOptimizationSettings(modified_context, query_info));
}
Before optimizing each child plan, the code removes any delayed materialization steps for CTEs that the outer query also references, preventing double-claiming.
4. Add regression tests to pin the fix:
+-- 1. The original repro: `View` child before `Distributed` child. No row can satisfy both
+-- `x IN (t)` and `x NOT IN (t)`.
+WITH t AS MATERIALIZED (SELECT number AS c FROM numbers(2))
+SELECT count() FROM merge(currentDatabase(), '^t_04811_(const|dist)$')
+WHERE (x IN (t)) AND (x NOT IN (t));
The new test file ensures the bug is fixed and does not regress.
From the review thread
No notable review discussion on this PR.
Takeaways
- When multiple parts of a query plan may reference or "claim" shared resources (like CTEs), ownership and gating must be coordinated at the correct (usually outermost) level to avoid race conditions and logical errors.
- Query optimizers that operate on subtrees independently can introduce subtle bugs if global context (such as shared CTEs) is not carefully managed and deduplicated.
- Regression tests for query planner bugs should cover both the presence and absence of the error, and should pin execution order and plan shape, since such bugs are often order-dependent.