From 00d1b4598aba11a82c406120eb86d38c6f3f5c6c Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Tue, 6 Oct 2026 10:59:11 +0900 Subject: [PATCH] Don't reprocess PHV copies within outer aggregates' arguments Since commit e28cf96e9, the copies of a query level's PlaceHolderVars that were pushed down into its subqueries are preprocessed by that level before its SubLinks are turned into SubPlans, so such a copy may already contain SubPlans by the time the subquery is planned. That commit taught build_subplan not to process the SubLinks of a PHV that is passed down as a SubPlan argument, but overlooked that such a copy can also appear within the arguments of an outer-level Aggref, GroupingFunc, or ReturningExpr. The SubLinks in those arguments are still processed when build_subplan collects the node, and that processing descended into the already-preprocessed PHV and tripped the no-SubPlan assertion in process_sublinks_mutator. To fix, don't descend into PHVs when processing the SubLinks in such a node's arguments. Any PHV found there is a pushed-down copy that its owning level has already preprocessed. Back-patch to v16, as with commit e28cf96e9. Author: Ayush Tiwari Reviewed-by: Richard Guo Discussion: https://postgr.es/m/CAJTYsWW8THbrnsmYnDqnUgg0PjGrqw2vNOWb8qx7eu191KPW3g@mail.gmail.com Backpatch-through: 16 --- src/backend/optimizer/plan/subselect.c | 20 ++++++++++++++++---- src/test/regress/expected/join.out | 25 +++++++++++++++++++++++++ src/test/regress/sql/join.sql | 6 ++++++ 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index 89221ee152d..e9b7afff86b 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -49,6 +49,7 @@ typedef struct process_sublinks_context { PlannerInfo *root; bool isTopQual; + bool skipPHVs; /* don't descend into PlaceHolderVars? */ } process_sublinks_context; typedef struct finalize_primnode_context @@ -363,11 +364,19 @@ build_subplan(PlannerInfo *root, Plan *plan, Path *path, * 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. + * already contain SubPlans. The same goes for any PlaceHolderVars + * within the arguments, so skip those. */ if (IsA(arg, Aggref) || IsA(arg, GroupingFunc)) - 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); @@ -1926,6 +1935,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); } @@ -1935,6 +1945,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; @@ -1967,11 +1978,12 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context) * at the outer query level; for an Aggref or GroupingFunc, they'll be * handled when build_subplan collects it into the arguments to be passed * down to the current subplan, while an outer PHV's expression has - * already been preprocessed by its owning level. + * already been preprocessed by its owning level. That also holds for a + * PHV within such an argument (skipPHVs). */ 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 67b8ed757bc..ac001f786e7 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -5925,6 +5925,31 @@ from (select case when false then remov.id else (select i41.f1) end as c1 Output: ((SubPlan 1)) (14 rows) +-- likewise, where the pushed-down PHV sits within an outer-level aggregate +explain (verbose, costs off) +select (select sum(ss1.c1) from int4_tbl i43) 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 2) + -> Nested Loop Left Join + Output: ((SubPlan 1)) + -> Seq Scan on public.int4_tbl i42 + Output: i42.f1 + -> Materialize + Output: ((SubPlan 1)) + -> Seq Scan on public.int4_tbl i41 + Output: (SubPlan 1) + SubPlan 1 + -> Result + Output: i41.f1 + SubPlan 2 + -> Seq Scan on public.int4_tbl i43 + Output: sum(((SubPlan 1))) +(16 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 2e3494e83af..e756113893d 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2164,6 +2164,12 @@ 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 pushed-down PHV sits within an outer-level aggregate +explain (verbose, costs off) +select (select sum(ss1.c1) from int4_tbl i43) as c2 +from (select (select i41.f1) as c1 from int4_tbl i41) ss1 + right join int4_tbl i42 on true; + -- 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)