Fix some new issues with planning of PlaceHolderVars.

In the wake of commit a16ef313f, we need to deal with more cases
involving PlaceHolderVars in NestLoopParams than we did before.

For one thing, a16ef313f was incorrect to suppose that we could
rely on the required-outer relids of the lefthand path to decide
placement of nestloop-parameter PHVs.  As Richard Guo argued at
the time, we must look at the required-outer relids of the join
path itself.

For another, we have to apply replace_nestloop_params() to such
a PHV's expression, in case it contains references to values that
will be supplied from NestLoopParams of higher-level nestloops.

For another, we need to be more careful about the phnullingrels
of the PHV than we were being.  identify_current_nestloop_params
only bothered to ensure that the phnullingrels didn't contain
"too many" relids, but now it has to be exact, because setrefs.c
will apply both NRM_SUBSET and NRM_SUPERSET checks in different
places.  We can compute the correct relids by determining the
set of outer joins that should be able to null the PHV and then
subtracting whatever's been applied at or below this join.
Do the same for plain Vars, too.  (This should make it possible
to use NRM_EQUAL to process nestloop params in setrefs.c, but
I won't risk making such a change in v18 now.)

Lastly, if a nestloop parameter PHV was pulled up out of a subquery
and it contains a subquery that was originally pushed down from this
query level, then that will still be represented as a SubLink, because
SS_process_sublinks won't recurse into outer PHVs, so it didn't get
transformed during expression preprocessing in the subquery.  We can
substitute the version of the PHV's expression appearing in its
PlaceHolderInfo to ensure that that preprocessing has happened.
(Seems like this processing sequence could stand to be redesigned,
but again, late in v18 development is not the time for that.)

It's not very clear to me why the old have_dangerous_phv join-order
restriction prevented us from seeing the last three of these problems.
But given the lack of field complaints, it must have done so.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18953-1c9883a9d4afeb30@postgresql.org

1 files changed, 27 insertions, 7 deletions

diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 8baf36ba4b7..0b61aef962c 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -4344,6 +4344,7 @@ create_nestloop_plan(PlannerInfo *root,
 	NestLoop   *join_plan;
 	Plan	   *outer_plan;
 	Plan	   *inner_plan;
+	Relids		outerrelids;
 	List	   *tlist = build_path_tlist(root, &best_path->jpath.path);
 	List	   *joinrestrictclauses = best_path->jpath.joinrestrictinfo;
 	List	   *joinclauses;
@@ -4374,8 +4375,8 @@ create_nestloop_plan(PlannerInfo *root,
 	outer_plan = create_plan_recurse(root, best_path->jpath.outerjoinpath, 0);
 
 	/* For a nestloop, include outer relids in curOuterRels for inner side */
-	root->curOuterRels = bms_union(root->curOuterRels,
-								   best_path->jpath.outerjoinpath->parent->relids);
+	outerrelids = best_path->jpath.outerjoinpath->parent->relids;
+	root->curOuterRels = bms_union(root->curOuterRels, outerrelids);
 
 	inner_plan = create_plan_recurse(root, best_path->jpath.innerjoinpath, 0);
 
@@ -4415,7 +4416,8 @@ create_nestloop_plan(PlannerInfo *root,
 	 * node, and remove them from root->curOuterParams.
 	 */
 	nestParams = identify_current_nestloop_params(root,
-												  best_path->jpath.outerjoinpath);
+												  outerrelids,
+												  PATH_REQ_OUTER((Path *) best_path));
 
 	/*
 	 * While nestloop parameters that are Vars had better be available from
@@ -4423,32 +4425,50 @@ create_nestloop_plan(PlannerInfo *root,
 	 * that are PHVs won't be.  In such cases we must add them to the
 	 * outer_plan's tlist, since the executor's NestLoopParam machinery
 	 * requires the params to be simple outer-Var references to that tlist.
+	 * (This is cheating a little bit, because the outer path's required-outer
+	 * relids might not be enough to allow evaluating such a PHV.  But in
+	 * practice, if we could have evaluated the PHV at the nestloop node, we
+	 * can do so in the outer plan too.)
 	 */
 	outer_tlist = outer_plan->targetlist;
 	outer_parallel_safe = outer_plan->parallel_safe;
 	foreach(lc, nestParams)
 	{
 		NestLoopParam *nlp = (NestLoopParam *) lfirst(lc);
+		PlaceHolderVar *phv;
 		TargetEntry *tle;
 
 		if (IsA(nlp->paramval, Var))
 			continue;			/* nothing to do for simple Vars */
-		if (tlist_member((Expr *) nlp->paramval, outer_tlist))
+		/* Otherwise it must be a PHV */
+		phv = castNode(PlaceHolderVar, nlp->paramval);
+
+		if (tlist_member((Expr *) phv, outer_tlist))
 			continue;			/* already available */
 
+		/*
+		 * It's possible that nestloop parameter PHVs selected to evaluate
+		 * here contain references to surviving root->curOuterParams items
+		 * (that is, they reference values that will be supplied by some
+		 * higher-level nestloop).  Those need to be converted to Params now.
+		 * Note: it's safe to do this after the tlist_member() check, because
+		 * equal() won't pay attention to phv->phexpr.
+		 */
+		phv->phexpr = (Expr *) replace_nestloop_params(root,
+													   (Node *) phv->phexpr);
+
 		/* Make a shallow copy of outer_tlist, if we didn't already */
 		if (outer_tlist == outer_plan->targetlist)
 			outer_tlist = list_copy(outer_tlist);
 		/* ... and add the needed expression */
-		tle = makeTargetEntry((Expr *) copyObject(nlp->paramval),
+		tle = makeTargetEntry((Expr *) copyObject(phv),
 							  list_length(outer_tlist) + 1,
 							  NULL,
 							  true);
 		outer_tlist = lappend(outer_tlist, tle);
 		/* ... and track whether tlist is (still) parallel-safe */
 		if (outer_parallel_safe)
-			outer_parallel_safe = is_parallel_safe(root,
-												   (Node *) nlp->paramval);
+			outer_parallel_safe = is_parallel_safe(root, (Node *) phv);
 	}
 	if (outer_tlist != outer_plan->targetlist)
 		outer_plan = change_plan_targetlist(outer_plan, outer_tlist,