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 <ayushtiwari.slg01@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWW8THbrnsmYnDqnUgg0PjGrqw2vNOWb8qx7eu191KPW3g@mail.gmail.com
Backpatch-through: 16

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
@@ -364,11 +365,19 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
 		 * 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);
@@ -1924,6 +1933,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);
 }
 
@@ -1933,6 +1943,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;
@@ -1965,11 +1976,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))

src/test/regress/expected/join.out

@@ -5926,6 +5926,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)

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)