From f17e2ce536ffe11487c7d286e8779cdb616f2a90 Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Fri, 25 Sep 2026 13:25:58 +0530 Subject: [PATCH v1] Don't reprocess PHV copies within outer-level aggregate arguments Commit e28cf96e9 arranged for each query level to preprocess the copies of its PlaceHolderVars that were pushed down into subqueries, so such a copy may already contain SubPlans by the time build_subplan collects it as an argument for a SubPlan. Accordingly, build_subplan stopped running SS_process_sublinks on a PlaceHolderVar argument. However, it still runs SS_process_sublinks on the arguments of an outer-level Aggref, GroupingFunc, or ReturningExpr, and those arguments can contain the same already-preprocessed PHV copies, as in select (select sum(ss.c) from t i offset 0) from (select (select s.a) as c from t s) ss right join t o on true; Walking into such a copy there trips the assertion in process_sublinks_mutator that it never sees a SubPlan. Builds without assertions just copy the SubPlan, which seems harmless. To fix, skip PlaceHolderVars when processing SubLinks within those arguments, just as for a bare PlaceHolderVar argument. Any other SubLinks in the arguments are still processed there, as before. Oversight in e28cf96e9, which was back-patched to v16. --- src/backend/optimizer/plan/subselect.c | 17 +++++++-- src/test/regress/expected/join.out | 53 ++++++++++++++++++++++++++ src/test/regress/sql/join.sql | 14 +++++++ 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index 5760b616813..1a8fb6e78a4 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -50,6 +50,7 @@ typedef struct process_sublinks_context { PlannerInfo *root; bool isTopQual; + bool skipPHVs; /* all PHVs are already preprocessed */ } process_sublinks_context; typedef struct finalize_primnode_context @@ -376,12 +377,20 @@ build_subplan(PlannerInfo *root, Plan *plan, Path *path, * comments for SS_replace_correlation_vars). Do that now. A * PlaceHolderVar needs no such treatment: subquery_planner already * preprocessed the PHVs of its owning level, so its expression is - * fully processed and may already contain SubPlans. + * fully processed and may already contain SubPlans. That's also true + * of any PlaceHolderVar within the arguments, so skip those. */ if (IsA(arg, Aggref) || IsA(arg, GroupingFunc) || IsA(arg, ReturningExpr)) - arg = SS_process_sublinks(root, arg, false); + { + process_sublinks_context context; + + context.root = root; + context.isTopQual = false; + context.skipPHVs = true; + arg = process_sublinks_mutator(arg, &context); + } splan->parParam = lappend_int(splan->parParam, pitem->paramId); splan->args = lappend(splan->args, arg); @@ -2217,6 +2226,7 @@ SS_process_sublinks(PlannerInfo *root, Node *expr, bool isQual) context.root = root; context.isTopQual = isQual; + context.skipPHVs = false; return process_sublinks_mutator(expr, &context); } @@ -2226,6 +2236,7 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context) process_sublinks_context locContext; locContext.root = context->root; + locContext.skipPHVs = context->skipPHVs; if (node == NULL) return NULL; @@ -2262,7 +2273,7 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context) */ if (IsA(node, PlaceHolderVar)) { - if (((PlaceHolderVar *) node)->phlevelsup > 0) + if (((PlaceHolderVar *) node)->phlevelsup > 0 || context->skipPHVs) return node; } else if (IsA(node, Aggref)) diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 94e158d0dc6..38c25e7be69 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -7430,6 +7430,59 @@ from (select case when false then remov.id else (select i41.f1) end as c1 Output: ((SubPlan expr_1)) (14 rows) +-- likewise, where the PHV copy is within the argument of an outer-level +-- aggregate, which is processed for SubLinks while building the SubPlan +explain (verbose, costs off) +select (select sum(ss1.c1) from int4_tbl i43 offset 0) as c2 +from (select (select i41.f1) as c1 from int4_tbl i41) ss1 + right join int4_tbl i42 on true; + QUERY PLAN +--------------------------------------------------- + Aggregate + Output: (SubPlan expr_2) + -> Nested Loop Left Join + Output: ((SubPlan expr_1)) + -> Seq Scan on public.int4_tbl i42 + Output: i42.f1 + -> Materialize + Output: ((SubPlan expr_1)) + -> Seq Scan on public.int4_tbl i41 + Output: (SubPlan expr_1) + SubPlan expr_1 + -> Result + Output: i41.f1 + SubPlan expr_2 + -> Seq Scan on public.int4_tbl i43 + Output: sum(((SubPlan expr_1))) +(16 rows) + +-- likewise, where the PHV copy is within an outer-level GROUPING() +explain (verbose, costs off) +select (select grouping(ss1.c1) from int4_tbl i43 offset 0) as c2 +from (select (select i41.f1) as c1 from int4_tbl i41) ss1 + right join int4_tbl i42 on true +group by ss1.c1; + QUERY PLAN +--------------------------------------------------- + HashAggregate + Output: (SubPlan expr_2), ((SubPlan expr_1)) + Group Key: ((SubPlan expr_1)) + -> Nested Loop Left Join + Output: ((SubPlan expr_1)) + -> Seq Scan on public.int4_tbl i42 + Output: i42.f1 + -> Materialize + Output: ((SubPlan expr_1)) + -> Seq Scan on public.int4_tbl i41 + Output: (SubPlan expr_1) + SubPlan expr_1 + -> Result + Output: i41.f1 + SubPlan expr_2 + -> Seq Scan on public.int4_tbl i43 + Output: GROUPING(((SubPlan expr_1))) +(17 rows) + -- likewise, where the pushed-down PHV's expression contains another PHV of -- the same level, which must not be preprocessed separately from its parent explain (verbose, costs off) diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 576a90dfc1b..70d7e393576 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2746,6 +2746,20 @@ from (select case when false then remov.id else (select i41.f1) end as c1 from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1 right join int4_tbl i42 on true; +-- likewise, where the PHV copy is within the argument of an outer-level +-- aggregate, which is processed for SubLinks while building the SubPlan +explain (verbose, costs off) +select (select sum(ss1.c1) from int4_tbl i43 offset 0) as c2 +from (select (select i41.f1) as c1 from int4_tbl i41) ss1 + right join int4_tbl i42 on true; + +-- likewise, where the PHV copy is within an outer-level GROUPING() +explain (verbose, costs off) +select (select grouping(ss1.c1) from int4_tbl i43 offset 0) as c2 +from (select (select i41.f1) as c1 from int4_tbl i41) ss1 + right join int4_tbl i42 on true +group by ss1.c1; + -- likewise, where the pushed-down PHV's expression contains another PHV of -- the same level, which must not be preprocessed separately from its parent explain (verbose, costs off) -- 2.34.1