Prevent overly-aggressive collapsing of joins to RTE_RESULT relations.

tglsfdc · tglsfdc · commit 6ea364e7e7d5 · 2019-12-14T13:49:15.000-05:00
The RTE_RESULT simplification logic added by commit 4be058f had a flaw: it would collapse out a RTE_RESULT that is due to compute a PlaceHolderVar, and reassign the PHV to the parent join level, even if another input relation of the join contained a lateral reference to the PHV. That can't work because the PHV would be computed too late. In practice it led to failures of internal sanity checks later in planning (either assertion failures or errors such as "failed to construct the join relation"). To fix, add code to check for the presence of such PHVs in relevant portions of the query tree. Notably, this required refactoring range_table_walker so that a caller could ask to walk individual RTEs not the whole list. (It might be a good idea to refactor range_table_mutator in the same way, if only to keep those functions looking similar; but I didn't do so here as it wasn't necessary for the bug fix.) This exercise also taught me that find_dependent_phvs(), as it stood, could only safely be used on the entire Query, not on subtrees. Adjust its API to reflect that; which in passing allows it to have a fast path for the common case of no PHVs anywhere. Per report from Will Leinweber. Back-patch to v12 where the bug was introduced. Discussion: https://postgr.es/m/CALLb-4xJMd4GZt2YCecMC95H-PafuWNKcmps4HLRx2NHNBfB4g@mail.gmail.com
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
@@ -2378,59 +2378,74 @@ range_table_walker(List *rtable,
 
 	foreach(rt, rtable)
 	{
-		RangeTblEntry *rte = (RangeTblEntry *) lfirst(rt);
+		RangeTblEntry *rte = lfirst_node(RangeTblEntry, rt);
 
-		/*
-		 * Walkers might need to examine the RTE node itself either before or
-		 * after visiting its contents (or, conceivably, both).  Note that if
-		 * you specify neither flag, the walker won't visit the RTE at all.
-		 */
-		if (flags & QTW_EXAMINE_RTES_BEFORE)
-			if (walker(rte, context))
-				return true;
+		if (range_table_entry_walker(rte, walker, context, flags))
+			return true;
+	}
+	return false;
+}
 
-		switch (rte->rtekind)
-		{
-			case RTE_RELATION:
-				if (walker(rte->tablesample, context))
-					return true;
-				break;
-			case RTE_SUBQUERY:
-				if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
-					if (walker(rte->subquery, context))
-						return true;
-				break;
-			case RTE_JOIN:
-				if (!(flags & QTW_IGNORE_JOINALIASES))
-					if (walker(rte->joinaliasvars, context))
-						return true;
-				break;
-			case RTE_FUNCTION:
-				if (walker(rte->functions, context))
-					return true;
-				break;
-			case RTE_TABLEFUNC:
-				if (walker(rte->tablefunc, context))
+/*
+ * Some callers even want to scan the expressions in individual RTEs.
+ */
+bool
+range_table_entry_walker(RangeTblEntry *rte,
+						 bool (*walker) (),
+						 void *context,
+						 int flags)
+{
+	/*
+	 * Walkers might need to examine the RTE node itself either before or
+	 * after visiting its contents (or, conceivably, both).  Note that if you
+	 * specify neither flag, the walker won't be called on the RTE at all.
+	 */
+	if (flags & QTW_EXAMINE_RTES_BEFORE)
+		if (walker(rte, context))
+			return true;
+
+	switch (rte->rtekind)
+	{
+		case RTE_RELATION:
+			if (walker(rte->tablesample, context))
+				return true;
+			break;
+		case RTE_SUBQUERY:
+			if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
+				if (walker(rte->subquery, context))
 					return true;
-				break;
-			case RTE_VALUES:
-				if (walker(rte->values_lists, context))
+			break;
+		case RTE_JOIN:
+			if (!(flags & QTW_IGNORE_JOINALIASES))
+				if (walker(rte->joinaliasvars, context))
 					return true;
-				break;
-			case RTE_CTE:
-			case RTE_NAMEDTUPLESTORE:
-			case RTE_RESULT:
-				/* nothing to do */
-				break;
-		}
+			break;
+		case RTE_FUNCTION:
+			if (walker(rte->functions, context))
+				return true;
+			break;
+		case RTE_TABLEFUNC:
+			if (walker(rte->tablefunc, context))
+				return true;
+			break;
+		case RTE_VALUES:
+			if (walker(rte->values_lists, context))
+				return true;
+			break;
+		case RTE_CTE:
+		case RTE_NAMEDTUPLESTORE:
+		case RTE_RESULT:
+			/* nothing to do */
+			break;
+	}
 
-		if (walker(rte->securityQuals, context))
+	if (walker(rte->securityQuals, context))
+		return true;
+
+	if (flags & QTW_EXAMINE_RTES_AFTER)
+		if (walker(rte, context))
 			return true;
 
-		if (flags & QTW_EXAMINE_RTES_AFTER)
-			if (walker(rte, context))
-				return true;
-	}
 	return false;
 }
 
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
@@ -120,7 +120,9 @@ static void reduce_outer_joins_pass2(Node *jtnode,
 static Node *remove_useless_results_recurse(PlannerInfo *root, Node *jtnode);
 static int	get_result_relid(PlannerInfo *root, Node *jtnode);
 static void remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc);
-static bool find_dependent_phvs(Node *node, int varno);
+static bool find_dependent_phvs(PlannerInfo *root, int varno);
+static bool find_dependent_phvs_in_jointree(PlannerInfo *root,
+											Node *node, int varno);
 static void substitute_phv_relids(Node *node,
 								  int varno, Relids subrelids);
 static void fix_append_rel_relids(List *append_rel_list, int varno,
@@ -2957,9 +2959,12 @@ reduce_outer_joins_pass2(Node *jtnode,
  * and not remove the RTE_RESULT: there is noplace else to evaluate the
  * PlaceHolderVar.  (That is, in such cases the RTE_RESULT *does* have output
  * columns.)  But if the RTE_RESULT is an immediate child of an inner join,
- * we can change the PlaceHolderVar's phrels so as to evaluate it at the
- * inner join instead.  This is OK because we really only care that PHVs are
- * evaluated above or below the correct outer joins.
+ * we can usually change the PlaceHolderVar's phrels so as to evaluate it at
+ * the inner join instead.  This is OK because we really only care that PHVs
+ * are evaluated above or below the correct outer joins.  We can't, however,
+ * postpone the evaluation of a PHV to above where it is used; so there are
+ * some checks below on whether output PHVs are laterally referenced in the
+ * other join input rel(s).
  *
  * We used to try to do this work as part of pull_up_subqueries() where the
  * potentially-optimizable cases get introduced; but it's way simpler, and
@@ -3021,8 +3026,11 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode)
 		/*
 		 * We can drop RTE_RESULT rels from the fromlist so long as at least
 		 * one child remains, since joining to a one-row table changes
-		 * nothing.  The easiest way to mechanize this rule is to modify the
-		 * list in-place.
+		 * nothing.  (But we can't drop a RTE_RESULT that computes PHV(s) that
+		 * are needed by some sibling.  The cleanup transformation below would
+		 * reassign the PHVs to be computed at the join, which is too late for
+		 * the sibling's use.)  The easiest way to mechanize this rule is to
+		 * modify the list in-place.
 		 */
 		foreach(cell, f->fromlist)
 		{
@@ -3035,12 +3043,14 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode)
 			lfirst(cell) = child;
 
 			/*
-			 * If it's an RTE_RESULT with at least one sibling, we can drop
-			 * it.  We don't yet know what the inner join's final relid set
-			 * will be, so postpone cleanup of PHVs etc till after this loop.
+			 * If it's an RTE_RESULT with at least one sibling, and no sibling
+			 * references dependent PHVs, we can drop it.  We don't yet know
+			 * what the inner join's final relid set will be, so postpone
+			 * cleanup of PHVs etc till after this loop.
 			 */
 			if (list_length(f->fromlist) > 1 &&
-				(varno = get_result_relid(root, child)) != 0)
+				(varno = get_result_relid(root, child)) != 0 &&
+				!find_dependent_phvs_in_jointree(root, (Node *) f, varno))
 			{
 				f->fromlist = foreach_delete_current(f->fromlist, cell);
 				result_relids = bms_add_member(result_relids, varno);
@@ -3091,8 +3101,18 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode)
 				 * the join with a FromExpr with just the other side; and if
 				 * the qual is empty (JOIN ON TRUE) then we can omit the
 				 * FromExpr as well.
+				 *
+				 * Just as in the FromExpr case, we can't simplify if the
+				 * other input rel references any PHVs that are marked as to
+				 * be evaluated at the RTE_RESULT rel, because we can't
+				 * postpone their evaluation in that case.  But we only have
+				 * to check this in cases where it's syntactically legal for
+				 * the other input to have a LATERAL reference to the
+				 * RTE_RESULT rel.  Only RHSes of inner and left joins are
+				 * allowed to have such refs.
 				 */
-				if ((varno = get_result_relid(root, j->larg)) != 0)
+				if ((varno = get_result_relid(root, j->larg)) != 0 &&
+					!find_dependent_phvs_in_jointree(root, j->rarg, varno))
 				{
 					remove_result_refs(root, varno, j->rarg);
 					if (j->quals)
@@ -3121,6 +3141,8 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode)
 				 * strength-reduced to a plain inner join, since each LHS row
 				 * necessarily has exactly one join partner.  So we can always
 				 * discard the RHS, much as in the JOIN_INNER case above.
+				 * (Again, the LHS could not contain a lateral reference to
+				 * the RHS.)
 				 *
 				 * Otherwise, it's still true that each LHS row should be
 				 * returned exactly once, and since the RHS returns no columns
@@ -3131,7 +3153,7 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode)
 				 */
 				if ((varno = get_result_relid(root, j->rarg)) != 0 &&
 					(j->quals == NULL ||
-					 !find_dependent_phvs((Node *) root->parse, varno)))
+					 !find_dependent_phvs(root, varno)))
 				{
 					remove_result_refs(root, varno, j->larg);
 					jtnode = j->larg;
@@ -3141,7 +3163,7 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode)
 				/* Mirror-image of the JOIN_LEFT case */
 				if ((varno = get_result_relid(root, j->larg)) != 0 &&
 					(j->quals == NULL ||
-					 !find_dependent_phvs((Node *) root->parse, varno)))
+					 !find_dependent_phvs(root, varno)))
 				{
 					remove_result_refs(root, varno, j->rarg);
 					jtnode = j->rarg;
@@ -3162,7 +3184,7 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode)
 				 */
 				if ((varno = get_result_relid(root, j->rarg)) != 0)
 				{
-					Assert(!find_dependent_phvs((Node *) root->parse, varno));
+					Assert(!find_dependent_phvs(root, varno));
 					remove_result_refs(root, varno, j->larg);
 					if (j->quals)
 						jtnode = (Node *)
@@ -3246,6 +3268,12 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc)
 /*
  * find_dependent_phvs - are there any PlaceHolderVars whose relids are
  * exactly the given varno?
+ *
+ * find_dependent_phvs should be used when we want to see if there are
+ * any such PHVs anywhere in the Query.  Another use-case is to see if
+ * a subtree of the join tree contains such PHVs; but for that, we have
+ * to look not only at the join tree nodes themselves but at the
+ * referenced RTEs.  For that, use find_dependent_phvs_in_jointree.
  */
 
 typedef struct
@@ -3292,20 +3320,65 @@ find_dependent_phvs_walker(Node *node,
 }
 
 static bool
-find_dependent_phvs(Node *node, int varno)
+find_dependent_phvs(PlannerInfo *root, int varno)
+{
+	find_dependent_phvs_context context;
+
+	/* If there are no PHVs anywhere, we needn't work hard */
+	if (root->glob->lastPHId == 0)
+		return false;
+
+	context.relids = bms_make_singleton(varno);
+	context.sublevels_up = 0;
+
+	return query_tree_walker(root->parse,
+							 find_dependent_phvs_walker,
+							 (void *) &context,
+							 0);
+}
+
+static bool
+find_dependent_phvs_in_jointree(PlannerInfo *root, Node *node, int varno)
 {
 	find_dependent_phvs_context context;
+	Relids		subrelids;
+	int			relid;
+
+	/* If there are no PHVs anywhere, we needn't work hard */
+	if (root->glob->lastPHId == 0)
+		return false;
 
 	context.relids = bms_make_singleton(varno);
 	context.sublevels_up = 0;
 
 	/*
-	 * Must be prepared to start with a Query or a bare expression tree.
+	 * See if the jointree fragment itself contains references (in join quals)
+	 */
+	if (find_dependent_phvs_walker(node, &context))
+		return true;
+
+	/*
+	 * Otherwise, identify the set of referenced RTEs (we can ignore joins,
+	 * since they should be flattened already, so their join alias lists no
+	 * longer matter), and tediously check each RTE.  We can ignore RTEs that
+	 * are not marked LATERAL, though, since they couldn't possibly contain
+	 * any cross-references to other RTEs.
 	 */
-	return query_or_expression_tree_walker(node,
-										   find_dependent_phvs_walker,
-										   (void *) &context,
-										   0);
+	subrelids = get_relids_in_jointree(node, false);
+	relid = -1;
+	while ((relid = bms_next_member(subrelids, relid)) >= 0)
+	{
+		RangeTblEntry *rte = rt_fetch(relid, root->parse->rtable);
+
+		if (rte->lateral &&
+			range_table_entry_walker(rte,
+									 find_dependent_phvs_walker,
+									 (void *) &context,
+									 0))
+			return true;
+	}
+
+	return false;
 }
 
 /*
diff --git a/src/include/nodes/nodeFuncs.h b/src/include/nodes/nodeFuncs.h
@@ -141,6 +141,9 @@ extern bool range_table_walker(List *rtable, bool (*walker) (),
 extern List *range_table_mutator(List *rtable, Node *(*mutator) (),
 								 void *context, int flags);
 
+extern bool range_table_entry_walker(RangeTblEntry *rte, bool (*walker) (),
+									 void *context, int flags);
+
 extern bool query_or_expression_tree_walker(Node *node, bool (*walker) (),
 											void *context, int flags);
 extern Node *query_or_expression_tree_mutator(Node *node, Node *(*mutator) (),
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
@@ -3242,6 +3242,32 @@ where t1.unique2 < 42 and t1.stringu1 > t2.stringu2;
       11 | WFAAAA   |       3 | LKIAAA
 (1 row)
 
+-- Here's a variant that we can't fold too aggressively, though,
+-- or we end up with noplace to evaluate the lateral PHV
+explain (verbose, costs off)
+select * from
+  (select 1 as x) ss1 left join (select 2 as y) ss2 on (true),
+  lateral (select ss2.y as z limit 1) ss3;
+        QUERY PLAN         
+---------------------------
+ Nested Loop
+   Output: 1, (2), ((2))
+   ->  Result
+         Output: 2
+   ->  Limit
+         Output: ((2))
+         ->  Result
+               Output: (2)
+(8 rows)
+
+select * from
+  (select 1 as x) ss1 left join (select 2 as y) ss2 on (true),
+  lateral (select ss2.y as z limit 1) ss3;
+ x | y | z 
+---+---+---
+ 1 | 2 | 2
+(1 row)
+
 --
 -- test inlining of immutable functions
 --
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
@@ -1018,6 +1018,16 @@ select t1.unique2, t1.stringu1, t2.unique1, t2.stringu2 from
   on (subq1.y1 = t2.unique1)
 where t1.unique2 < 42 and t1.stringu1 > t2.stringu2;
 
+-- Here's a variant that we can't fold too aggressively, though,
+-- or we end up with noplace to evaluate the lateral PHV
+explain (verbose, costs off)
+select * from
+  (select 1 as x) ss1 left join (select 2 as y) ss2 on (true),
+  lateral (select ss2.y as z limit 1) ss3;
+select * from
+  (select 1 as x) ss1 left join (select 2 as y) ss2 on (true),
+  lateral (select ss2.y as z limit 1) ss3;
+
 --
 -- test inlining of immutable functions
 --
