Fix preprocessing of PHV copies pushed down into subqueries

Commit e28cf96e9 preprocessed the copies of a query level's
PlaceHolderVars that were pushed down into its subqueries in a single
walk at the start of subquery_planner.  That was wrong in two ways.

First, such copies can also be inserted later, when a join alias Var
of the outer level is expanded within a LATERAL subquery or a
SubLink's subselect and the alias expression contains a PHV.  Those
copies were never preprocessed, so a SubLink within them survived into
the subquery's lateral references or into a SubPlan's argument list,
tripping an assertion in identify_current_nestloop_params or failing
in cost_qual_eval.

Second, when a pushed-down copy directly contains another PHV of the
same level, the walk preprocessed the inner one first and then the
outer one, so the outer one's preprocessing ran into the SubPlans just
created in the inner one and tripped an assertion in
flatten_join_alias_vars.

To fix, preprocess the copies right after join alias expansion
instead: within preprocess_expression, just before SubLinks are turned
into SubPlans, and for LATERAL subquery RTEs, right after their
aliases are flattened.  Preprocessing a copy handles everything within
it, including any further copies nested inside its SubLinks, so the
walker no longer looks inside a copy.

Back-patch to v16, as with commit e28cf96e9.

Reported-by: Fujii Masao <masao.fujii@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwH-acRnAk1=Ki4EiAJEt67+Rz=_jyg7pbP_c6BK4oHZ4Q@mail.gmail.com
Backpatch-through: 16

src/backend/optimizer/plan/planner.c

@@ -138,10 +138,10 @@ typedef struct
 
 /* Local functions */
 static Node *preprocess_expression(PlannerInfo *root, Node *expr, int kind);
-static void preprocess_subquery_phvs(PlannerInfo *root);
+static void preprocess_qual_conditions(PlannerInfo *root, Node *jtnode);
+static void preprocess_subquery_phvs(PlannerInfo *root, Node *node);
 static bool preprocess_subquery_phvs_walker(Node *node,
 											preprocess_subquery_phvs_context *context);
-static void preprocess_qual_conditions(PlannerInfo *root, Node *jtnode);
 static void grouping_planner(PlannerInfo *root, double tuple_fraction);
 static grouping_sets_data *preprocess_grouping_sets(PlannerInfo *root);
 static List *remap_to_groupclause_idx(List *groupClause, List *gsets,
@@ -830,14 +830,6 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
 	 */
 	root->hasHavingQual = (parse->havingQual != NULL);
 
-	/*
-	 * Preprocess any PlaceHolderVars of our level that were pushed down into
-	 * subqueries.  This must happen before anything can consume those copies,
-	 * in particular before SubLinks below are turned into SubPlans.
-	 */
-	if (root->glob->lastPHId != 0)
-		preprocess_subquery_phvs(root);
-
 	/*
 	 * Do expression preprocessing on targetlist and quals, as well as other
 	 * random expressions in the querytree.  Note that we do not need to
@@ -959,6 +951,14 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
 				rte->subquery = (Query *)
 					flatten_join_alias_vars(root, root->parse,
 											(Node *) rte->subquery);
+
+			/*
+			 * Likewise for copies of our PlaceHolderVars in the subquery.
+			 * This must be done after the alias expansion above, which can
+			 * insert such copies.
+			 */
+			if (rte->lateral && root->glob->lastPHId != 0)
+				preprocess_subquery_phvs(root, (Node *) rte->subquery);
 		}
 		else if (rte->rtekind == RTE_FUNCTION)
 		{
@@ -1203,6 +1203,15 @@ preprocess_expression(PlannerInfo *root, Node *expr, int kind)
 		convert_saop_to_hashed_saop(expr);
 	}
 
+	/*
+	 * Preprocess any copies of our PlaceHolderVars within SubLink subselects.
+	 * This must be done after join alias expansion, which can insert such
+	 * copies, and before the SubLinks are turned into SubPlans, which collect
+	 * those copies as SubPlan arguments.
+	 */
+	if (root->parse->hasSubLinks && root->glob->lastPHId != 0)
+		preprocess_subquery_phvs(root, expr);
+
 	/* Expand SubLinks to SubPlans */
 	if (root->parse->hasSubLinks)
 		expr = SS_process_sublinks(root, expr, (kind == EXPRKIND_QUAL));
@@ -1286,24 +1295,25 @@ preprocess_phv_expression(PlannerInfo *root, Expr *expr)
 /*
  * preprocess_subquery_phvs
  *		Preprocess copies of this level's PlaceHolderVars that were pushed
- *		down into subqueries.
+ *		down into subqueries within the given tree.
  *
  * When a subquery (a LATERAL RTE or a SubLink's subselect) references a
  * pulled-up output that must be wrapped in a PlaceHolderVar, the PHV
  * expression is pushed down into the subquery.  The subquery's own planning
  * leaves that copy alone, since it belongs to our level, so we need to
  * preprocess it.  We modify the PHVs in place, temporarily adjusting each to
- * our level, and handle nested copies innermost-first.
+ * our level.  Preprocessing a copy's expression takes care of everything
+ * within it, including any further copies nested inside SubLinks there, so
+ * we don't look inside a copy ourselves.
  */
 static void
-preprocess_subquery_phvs(PlannerInfo *root)
+preprocess_subquery_phvs(PlannerInfo *root, Node *node)
 {
 	preprocess_subquery_phvs_context context;
 
 	context.root = root;
 	context.sublevels_up = 0;
-	(void) query_tree_walker(root->parse, preprocess_subquery_phvs_walker,
-							 &context, 0);
+	(void) preprocess_subquery_phvs_walker(node, &context);
 }
 
 static bool
@@ -1327,9 +1337,9 @@ preprocess_subquery_phvs_walker(Node *node,
 	{
 		PlaceHolderVar *phv = (PlaceHolderVar *) node;
 
-		/* Handle any nested copies within the expression first */
-		(void) expression_tree_walker(node, preprocess_subquery_phvs_walker,
-									  context);
+		/* A PHV of an upper level can't contain anything of our level */
+		if (phv->phlevelsup > context->sublevels_up)
+			return false;
 
 		/*
 		 * Is this a copy of one of our PHVs that is pushed down into a
@@ -1347,8 +1357,10 @@ preprocess_subquery_phvs_walker(Node *node,
 			expr = preprocess_expression(context->root, expr, EXPRKIND_PHV);
 			IncrementVarSublevelsUp(expr, levelsup, 0);
 			phv->phexpr = (Expr *) expr;
+			return false;
 		}
-		return false;
+
+		/* Otherwise, it's ours or a lower level's; look inside it */
 	}
 	return expression_tree_walker(node, preprocess_subquery_phvs_walker,
 								  context);

src/backend/optimizer/plan/subselect.c

@@ -1962,9 +1962,10 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
 	/*
 	 * Don't recurse into the arguments of an outer PHV, Aggref or
 	 * GroupingFunc here.  Any SubLinks in the arguments have to be dealt with
-	 * at the outer query level; they'll be handled when build_subplan
-	 * collects the PHV, Aggref or GroupingFunc into the arguments to be
-	 * passed down to the current subplan.
+	 * 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.
 	 */
 	if (IsA(node, PlaceHolderVar))
 	{

src/backend/optimizer/util/var.c

@@ -868,11 +868,10 @@ flatten_join_alias_vars_mutator(Node *node,
 			return node;		/* no need to copy, really */
 
 		/*
-		 * A pushed-down copy of a target-level PHV was already preprocessed
-		 * by preprocess_subquery_phvs, so it may contain SubPlans and holds
-		 * no unflattened target-level aliases; shallow-copy it instead of
-		 * recursing.  Any other PHV is recursed into, as it may still hold
-		 * target-level aliases.
+		 * A pushed-down copy of a target-level PHV is preprocessed by
+		 * preprocess_subquery_phvs, which handles its target-level aliases
+		 * and may leave SubPlans in it; so don't recurse into it.  Any other
+		 * PHV may still hold target-level aliases, so recurse.
 		 */
 		if (phv->phlevelsup == context->sublevels_up &&
 			context->sublevels_up > 0)

src/test/regress/expected/join.out

@@ -5854,24 +5854,24 @@ from (select case when false then remov.id else (select i41.f1) end as c1
                        QUERY PLAN                        
 ---------------------------------------------------------
  Nested Loop
-   Output: ((SubPlan 2)), (((SubPlan 2)))
+   Output: ((SubPlan 1)), (((SubPlan 1)))
    ->  Nested Loop Left Join
-         Output: ((SubPlan 2))
+         Output: ((SubPlan 1))
          Join Filter: false
          ->  Seq Scan on public.int4_tbl i42
                Output: i42.f1
          ->  Result
-               Output: (SubPlan 2)
+               Output: (SubPlan 1)
                One-Time Filter: false
-               SubPlan 2
+               SubPlan 1
                  ->  Result
                        Output: f1
    ->  Memoize
-         Output: (((SubPlan 2)))
-         Cache Key: ((SubPlan 2))
+         Output: (((SubPlan 1)))
+         Cache Key: ((SubPlan 1))
          Cache Mode: binary
          ->  Nested Loop
-               Output: ((SubPlan 2))
+               Output: ((SubPlan 1))
                ->  Seq Scan on public.int4_tbl i43
                      Output: i43.f1
                ->  Materialize
@@ -5926,6 +5926,134 @@ 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's expression contains another PHV of
+-- the same level, which must not be preprocessed separately from its parent
+explain (verbose, costs off)
+select ss3.c2
+from int4_tbl i41
+     left join (select coalesce(ss1.c1, 0) as c1
+                from int4_tbl i42
+                     left join (select (select i43.f1) as c1
+                                from int4_tbl i43) ss1 on true) ss2
+       on true,
+     lateral (select ss2.c1 as c2 from int4_tbl i44 offset 0) ss3;
+                          QUERY PLAN                           
+---------------------------------------------------------------
+ Nested Loop
+   Output: ((COALESCE(((SubPlan 1)), 0)))
+   ->  Nested Loop Left Join
+         Output: (COALESCE(((SubPlan 1)), 0))
+         ->  Seq Scan on public.int4_tbl i41
+               Output: i41.f1
+         ->  Materialize
+               Output: (COALESCE(((SubPlan 1)), 0))
+               ->  Nested Loop Left Join
+                     Output: COALESCE(((SubPlan 1)), 0)
+                     ->  Seq Scan on public.int4_tbl i42
+                           Output: i42.f1
+                     ->  Materialize
+                           Output: ((SubPlan 1))
+                           ->  Seq Scan on public.int4_tbl i43
+                                 Output: (SubPlan 1)
+                                 SubPlan 1
+                                   ->  Result
+                                         Output: i43.f1
+   ->  Memoize
+         Output: ((COALESCE(((SubPlan 1)), 0)))
+         Cache Key: (COALESCE(((SubPlan 1)), 0))
+         Cache Mode: binary
+         ->  Seq Scan on public.int4_tbl i44
+               Output: (COALESCE(((SubPlan 1)), 0))
+(25 rows)
+
+-- likewise, where the PHV copy is only inserted into the LATERAL subquery by
+-- expanding a join alias Var of the outer level
+explain (verbose, costs off)
+select ss2.c2
+from ((select (select i41.f1) as c1 from int4_tbl i41) ss1
+      full join int4_tbl i42(c1) using (c1)) j,
+     lateral (select j.c1 as c2 from int4_tbl i43 offset 0) ss2;
+                    QUERY PLAN                     
+---------------------------------------------------
+ Nested Loop
+   Output: (COALESCE(((SubPlan 1)), i42.c1))
+   ->  Hash Full Join
+         Output: ((SubPlan 1)), i42.c1
+         Hash Cond: (((SubPlan 1)) = i42.c1)
+         ->  Seq Scan on public.int4_tbl i41
+               Output: (SubPlan 1)
+               SubPlan 1
+                 ->  Result
+                       Output: i41.f1
+         ->  Hash
+               Output: i42.c1
+               ->  Seq Scan on public.int4_tbl i42
+                     Output: i42.c1
+   ->  Seq Scan on public.int4_tbl i43
+         Output: COALESCE(((SubPlan 1)), i42.c1)
+(16 rows)
+
+-- likewise, where the join alias is expanded within a SubLink's subselect
+explain (verbose, costs off)
+select (select j.c1 from int4_tbl i43 offset 0) as c2
+from ((select (select i41.f1) as c1 from int4_tbl i41) ss1
+      full join int4_tbl i42(c1) using (c1)) j;
+                    QUERY PLAN                     
+---------------------------------------------------
+ Hash Full Join
+   Output: (SubPlan 2)
+   Hash Cond: (((SubPlan 1)) = i42.c1)
+   ->  Seq Scan on public.int4_tbl i41
+         Output: (SubPlan 1)
+         SubPlan 1
+           ->  Result
+                 Output: i41.f1
+   ->  Hash
+         Output: i42.c1
+         ->  Seq Scan on public.int4_tbl i42
+               Output: i42.c1
+   SubPlan 2
+     ->  Seq Scan on public.int4_tbl i43
+           Output: COALESCE(((SubPlan 1)), i42.c1)
+(15 rows)
+
+-- likewise, where the SubLink holding the copy ends up in the translated
+-- Vars of an appendrel child pulled up from a LATERAL UNION ALL subquery
+explain (verbose, costs off)
+select ss2.c2
+from (select case when false then remov.id end as c1
+      from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1
+     right join int4_tbl i42 on true,
+     lateral ((select (select ss1.c1) as c2) union all (select (select ss1.c1))) ss2;
+                     QUERY PLAN                      
+-----------------------------------------------------
+ Nested Loop
+   Output: ((SubPlan 1))
+   ->  Nested Loop Left Join
+         Output: (NULL::integer)
+         ->  Seq Scan on public.int4_tbl i42
+               Output: i42.f1
+         ->  Materialize
+               Output: (NULL::integer)
+               ->  Seq Scan on public.int4_tbl i41
+                     Output: NULL::integer
+   ->  Memoize
+         Output: ((SubPlan 1))
+         Cache Key: (NULL::integer), (NULL::integer)
+         Cache Mode: binary
+         ->  Append
+               ->  Result
+                     Output: (SubPlan 1)
+                     SubPlan 1
+                       ->  Result
+                             Output: (NULL::integer)
+               ->  Result
+                     Output: (SubPlan 2)
+                     SubPlan 2
+                       ->  Result
+                             Output: (NULL::integer)
+(25 rows)
+
 -- More tests of correct placement of pseudoconstant quals
 -- simple constant-false condition
 explain (costs off)

src/test/regress/sql/join.sql

@@ -2164,6 +2164,41 @@ 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's expression contains another PHV of
+-- the same level, which must not be preprocessed separately from its parent
+explain (verbose, costs off)
+select ss3.c2
+from int4_tbl i41
+     left join (select coalesce(ss1.c1, 0) as c1
+                from int4_tbl i42
+                     left join (select (select i43.f1) as c1
+                                from int4_tbl i43) ss1 on true) ss2
+       on true,
+     lateral (select ss2.c1 as c2 from int4_tbl i44 offset 0) ss3;
+
+-- likewise, where the PHV copy is only inserted into the LATERAL subquery by
+-- expanding a join alias Var of the outer level
+explain (verbose, costs off)
+select ss2.c2
+from ((select (select i41.f1) as c1 from int4_tbl i41) ss1
+      full join int4_tbl i42(c1) using (c1)) j,
+     lateral (select j.c1 as c2 from int4_tbl i43 offset 0) ss2;
+
+-- likewise, where the join alias is expanded within a SubLink's subselect
+explain (verbose, costs off)
+select (select j.c1 from int4_tbl i43 offset 0) as c2
+from ((select (select i41.f1) as c1 from int4_tbl i41) ss1
+      full join int4_tbl i42(c1) using (c1)) j;
+
+-- likewise, where the SubLink holding the copy ends up in the translated
+-- Vars of an appendrel child pulled up from a LATERAL UNION ALL subquery
+explain (verbose, costs off)
+select ss2.c2
+from (select case when false then remov.id end as c1
+      from int4_tbl i41 left join a remov on i41.f1 = remov.id) ss1
+     right join int4_tbl i42 on true,
+     lateral ((select (select ss1.c1) as c2) union all (select (select ss1.c1))) ss2;
+
 -- More tests of correct placement of pseudoconstant quals
 
 -- simple constant-false condition