Avoid mislabeling of lateral references, redux.

tglsfdc · tglsfdc · commit 78883cd90511 · 2024-11-30T12:42:20.000-05:00
As I'd feared, commit 5c9d863 was still a few bricks shy of a load. We can't just leave pulled-up lateral-reference Vars with no new nullingrels: we have to carefully compute what subset of the to-be-replaced Var's nullingrels apply to them, else we still get "wrong varnullingrels" errors. This is a bit tedious, but it looks like we can use the nullingrel data this patch computes for other purposes, enabling better optimization. We don't want to inject unnecessary plan changes into stable branches though, so leave that idea for a later HEAD-only patch. Patch by me, but thanks to Richard Guo for devising a test case that broke 5c9d863, and for preliminary investigation about how to fix it. As before, back-patch to v16. Discussion: https://postgr.es/m/E1tGn4j-0003zi-MP@gemulon.postgresql.org
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
@@ -42,13 +42,26 @@
 #include "rewrite/rewriteManip.h"
 
 
+typedef struct nullingrel_info
+{
+	/*
+	 * For each leaf RTE, nullingrels[rti] is the set of relids of outer joins
+	 * that potentially null that RTE.
+	 */
+	Relids	   *nullingrels;
+	/* Length of range table (maximum index in nullingrels[]) */
+	int			rtlength;		/* used only for assertion checks */
+} nullingrel_info;
+
 typedef struct pullup_replace_vars_context
 {
 	PlannerInfo *root;
 	List	   *targetlist;		/* tlist of subquery being pulled up */
 	RangeTblEntry *target_rte;	/* RTE of subquery */
 	Relids		relids;			/* relids within subquery, as numbered after
 								 * pullup (set only if target_rte->lateral) */
+	nullingrel_info *nullinfo;	/* per-RTE nullingrel info (set only if
+								 * target_rte->lateral) */
 	bool	   *outer_hasSubLinks;	/* -> outer query's hasSubLinks */
 	int			varno;			/* varno of subquery */
 	bool		wrap_non_vars;	/* do we need all non-Var outputs to be PHVs? */
@@ -142,6 +155,9 @@ static void substitute_phv_relids(Node *node,
 static void fix_append_rel_relids(PlannerInfo *root, int varno,
 								  Relids subrelids);
 static Node *find_jointree_node_for_rel(Node *jtnode, int relid);
+static nullingrel_info *get_nullingrels(Query *parse);
+static void get_nullingrels_recurse(Node *jtnode, Relids upper_nullingrels,
+									nullingrel_info *info);
 
 
 /*
@@ -1259,10 +1275,16 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
 	rvcontext.targetlist = subquery->targetList;
 	rvcontext.target_rte = rte;
 	if (rte->lateral)
+	{
 		rvcontext.relids = get_relids_in_jointree((Node *) subquery->jointree,
 												  true, true);
-	else						/* won't need relids */
+		rvcontext.nullinfo = get_nullingrels(parse);
+	}
+	else						/* won't need these values */
+	{
 		rvcontext.relids = NULL;
+		rvcontext.nullinfo = NULL;
+	}
 	rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
 	rvcontext.varno = varno;
 	/* this flag will be set below, if needed */
@@ -1724,6 +1746,9 @@ is_simple_subquery(PlannerInfo *root, Query *subquery, RangeTblEntry *rte,
 		 * such refs to be wrapped in PlaceHolderVars, even when they're below
 		 * the nearest outer join?	But it's a pretty hokey usage, so not
 		 * clear this is worth sweating over.)
+		 *
+		 * If you change this, see also the comments about lateral references
+		 * in pullup_replace_vars_callback().
 		 */
 		if (lowest_outer_join != NULL)
 		{
@@ -1808,7 +1833,8 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte)
 	rvcontext.root = root;
 	rvcontext.targetlist = tlist;
 	rvcontext.target_rte = rte;
-	rvcontext.relids = NULL;
+	rvcontext.relids = NULL;	/* can't be any lateral references here */
+	rvcontext.nullinfo = NULL;
 	rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
 	rvcontext.varno = varno;
 	rvcontext.wrap_non_vars = false;
@@ -1970,9 +1996,10 @@ pull_up_constant_function(PlannerInfo *root, Node *jtnode,
 	/*
 	 * Since this function was reduced to a Const, it doesn't contain any
 	 * lateral references, even if it's marked as LATERAL.  This means we
-	 * don't need to fill relids.
+	 * don't need to fill relids or nullinfo.
 	 */
 	rvcontext.relids = NULL;
+	rvcontext.nullinfo = NULL;
 
 	rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
 	rvcontext.varno = ((RangeTblRef *) jtnode)->rtindex;
@@ -2681,9 +2708,52 @@ pullup_replace_vars_callback(Var *var,
 		{
 			/*
 			 * There should be Vars/PHVs within the expression that we can
-			 * modify.  Per above discussion, modify only Vars/PHVs of the
-			 * subquery, not lateral references.
+			 * modify.  Vars/PHVs of the subquery should have the full
+			 * var->varnullingrels added to them, but if there are lateral
+			 * references within the expression, those must be marked with
+			 * only the nullingrels that potentially apply to them.  (This
+			 * corresponds to the fact that the expression will now be
+			 * evaluated at the join level of the Var that we are replacing:
+			 * the lateral references may have bubbled up through fewer outer
+			 * joins than the subquery's Vars have.  Per the discussion above,
+			 * we'll still get the right answers.)  That relid set could be
+			 * different for different lateral relations, so we have to do
+			 * this work for each one.
+			 *
+			 * (Currently, the restrictions in is_simple_subquery() mean that
+			 * at most we have to remove the lowest outer join's relid from
+			 * the nullingrels of a lateral reference.  However, we might
+			 * relax those restrictions someday, so let's do this right.)
 			 */
+			if (rcon->target_rte->lateral)
+			{
+				nullingrel_info *nullinfo = rcon->nullinfo;
+				Relids		lvarnos;
+				int			lvarno;
+
+				/*
+				 * Identify lateral varnos used within newnode.  We must do
+				 * this before injecting var->varnullingrels into the tree.
+				 */
+				lvarnos = pull_varnos(rcon->root, newnode);
+				lvarnos = bms_del_members(lvarnos, rcon->relids);
+				/* For each one, add relevant nullingrels if any */
+				lvarno = -1;
+				while ((lvarno = bms_next_member(lvarnos, lvarno)) >= 0)
+				{
+					Relids		lnullingrels;
+
+					Assert(lvarno > 0 && lvarno <= nullinfo->rtlength);
+					lnullingrels = bms_intersect(var->varnullingrels,
+												 nullinfo->nullingrels[lvarno]);
+					if (!bms_is_empty(lnullingrels))
+						newnode = add_nulling_relids(newnode,
+													 bms_make_singleton(lvarno),
+													 lnullingrels);
+				}
+			}
+
+			/* Finally, deal with Vars/PHVs of the subquery itself */
 			newnode = add_nulling_relids(newnode,
 										 rcon->relids,
 										 var->varnullingrels);
@@ -4121,3 +4191,94 @@ find_jointree_node_for_rel(Node *jtnode, int relid)
 			 (int) nodeTag(jtnode));
 	return NULL;
 }
+
+/*
+ * get_nullingrels: collect info about which outer joins null which relations
+ *
+ * The result struct contains, for each leaf relation used in the query,
+ * the set of relids of outer joins that potentially null that rel.
+ */
+static nullingrel_info *
+get_nullingrels(Query *parse)
+{
+	nullingrel_info *result = palloc_object(nullingrel_info);
+
+	result->rtlength = list_length(parse->rtable);
+	result->nullingrels = palloc0_array(Relids, result->rtlength + 1);
+	get_nullingrels_recurse((Node *) parse->jointree, NULL, result);
+	return result;
+}
+
+/*
+ * Recursive guts of get_nullingrels().
+ *
+ * Note: at any recursion level, the passed-down upper_nullingrels must be
+ * treated as a constant, but it can be stored directly into *info
+ * if we're at leaf level.  Upper recursion levels do not free their mutated
+ * copies of the nullingrels, because those are probably referenced by
+ * at least one leaf rel.
+ */
+static void
+get_nullingrels_recurse(Node *jtnode, Relids upper_nullingrels,
+						nullingrel_info *info)
+{
+	if (jtnode == NULL)
+		return;
+	if (IsA(jtnode, RangeTblRef))
+	{
+		int			varno = ((RangeTblRef *) jtnode)->rtindex;
+
+		Assert(varno > 0 && varno <= info->rtlength);
+		info->nullingrels[varno] = upper_nullingrels;
+	}
+	else if (IsA(jtnode, FromExpr))
+	{
+		FromExpr   *f = (FromExpr *) jtnode;
+		ListCell   *l;
+
+		foreach(l, f->fromlist)
+		{
+			get_nullingrels_recurse(lfirst(l), upper_nullingrels, info);
+		}
+	}
+	else if (IsA(jtnode, JoinExpr))
+	{
+		JoinExpr   *j = (JoinExpr *) jtnode;
+		Relids		local_nullingrels;
+
+		switch (j->jointype)
+		{
+			case JOIN_INNER:
+				get_nullingrels_recurse(j->larg, upper_nullingrels, info);
+				get_nullingrels_recurse(j->rarg, upper_nullingrels, info);
+				break;
+			case JOIN_LEFT:
+			case JOIN_SEMI:
+			case JOIN_ANTI:
+				local_nullingrels = bms_add_member(bms_copy(upper_nullingrels),
+												   j->rtindex);
+				get_nullingrels_recurse(j->larg, upper_nullingrels, info);
+				get_nullingrels_recurse(j->rarg, local_nullingrels, info);
+				break;
+			case JOIN_FULL:
+				local_nullingrels = bms_add_member(bms_copy(upper_nullingrels),
+												   j->rtindex);
+				get_nullingrels_recurse(j->larg, local_nullingrels, info);
+				get_nullingrels_recurse(j->rarg, local_nullingrels, info);
+				break;
+			case JOIN_RIGHT:
+				local_nullingrels = bms_add_member(bms_copy(upper_nullingrels),
+												   j->rtindex);
+				get_nullingrels_recurse(j->larg, local_nullingrels, info);
+				get_nullingrels_recurse(j->rarg, upper_nullingrels, info);
+				break;
+			default:
+				elog(ERROR, "unrecognized join type: %d",
+					 (int) j->jointype);
+				break;
+		}
+	}
+	else
+		elog(ERROR, "unrecognized node type: %d",
+			 (int) nodeTag(jtnode));
+}
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
@@ -1797,6 +1797,57 @@ order by t1.ten;
    9 | 18000
 (10 rows)
 
+explain (verbose, costs off)
+select t1.q1, x from
+  int8_tbl t1 left join
+  (int8_tbl t2 left join
+   lateral (select t2.q1+t3.q1 as x, * from int8_tbl t3) t3 on t2.q2 = t3.q2)
+  on t1.q2 = t2.q2
+order by 1, 2;
+                       QUERY PLAN                       
+--------------------------------------------------------
+ Sort
+   Output: t1.q1, ((t2.q1 + t3.q1))
+   Sort Key: t1.q1, ((t2.q1 + t3.q1))
+   ->  Hash Left Join
+         Output: t1.q1, (t2.q1 + t3.q1)
+         Hash Cond: (t2.q2 = t3.q2)
+         ->  Hash Left Join
+               Output: t1.q1, t2.q1, t2.q2
+               Hash Cond: (t1.q2 = t2.q2)
+               ->  Seq Scan on public.int8_tbl t1
+                     Output: t1.q1, t1.q2
+               ->  Hash
+                     Output: t2.q1, t2.q2
+                     ->  Seq Scan on public.int8_tbl t2
+                           Output: t2.q1, t2.q2
+         ->  Hash
+               Output: t3.q1, t3.q2
+               ->  Seq Scan on public.int8_tbl t3
+                     Output: t3.q1, t3.q2
+(19 rows)
+
+select t1.q1, x from
+  int8_tbl t1 left join
+  (int8_tbl t2 left join
+   lateral (select t2.q1+t3.q1 as x, * from int8_tbl t3) t3 on t2.q2 = t3.q2)
+  on t1.q2 = t2.q2
+order by 1, 2;
+        q1        |        x         
+------------------+------------------
+              123 |              246
+              123 |              246
+              123 | 4567890123456912
+              123 | 4567890123456912
+              123 | 9135780246913578
+ 4567890123456789 |              246
+ 4567890123456789 | 4567890123456912
+ 4567890123456789 | 4567890123456912
+ 4567890123456789 | 9135780246913578
+ 4567890123456789 | 9135780246913578
+ 4567890123456789 | 9135780246913578
+(11 rows)
+
 --
 -- Tests for CTE inlining behavior
 --
diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql
@@ -924,6 +924,21 @@ select t1.ten, sum(x) from
 group by t1.ten
 order by t1.ten;
 
+explain (verbose, costs off)
+select t1.q1, x from
+  int8_tbl t1 left join
+  (int8_tbl t2 left join
+   lateral (select t2.q1+t3.q1 as x, * from int8_tbl t3) t3 on t2.q2 = t3.q2)
+  on t1.q2 = t2.q2
+order by 1, 2;
+
+select t1.q1, x from
+  int8_tbl t1 left join
+  (int8_tbl t2 left join
+   lateral (select t2.q1+t3.q1 as x, * from int8_tbl t3) t3 on t2.q2 = t3.q2)
+  on t1.q2 = t2.q2
+order by 1, 2;
+
 --
 -- Tests for CTE inlining behavior
 --
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
@@ -3639,6 +3639,7 @@ nodeitem
 normal_rand_fctx
 nsphash_hash
 ntile_context
+nullingrel_info
 numeric
 object_access_hook_type
 object_access_hook_type_str
