Fix up join removal's interaction with PlaceHolderVars.

tglsfdc · tglsfdc · commit cad56920513e · 2023-02-06T15:45:03.000-05:00
The portion of join_is_removable() that checks PlaceHolderVars can be made a little more accurate and intelligible than it was. The key point is that we can allow join removal even if a PHV mentions the target rel in ph_eval_at, if that mention was only added as a consequence of forcing the PHV up to a join level that's at/above the outer join we're trying to get rid of. We can check that by testing for the OJ's relid appearing in ph_eval_at, indicating that it's supposed to be evaluated after the outer join, plus the existing test that the contained expression doesn't actually mention the target rel. While here, add an explicit check that there'll be something left in ph_eval_at after we remove the target rel and OJ relid. There is an Assert later on about that, and I'm not too sure that the case could happen for a PHV satisfying the other constraints, but let's just check. (There was previously a bms_is_subset test that meant to cover this risk, but it's broken now because it doesn't account for the fact that we'll also remove the OJ relid.) The real reason for revisiting this code though is that the Assert I left behind in 8538519 turns out to be easily reachable, because if a PHV of this sort appears in an upper-level qual clause then that clause's clause_relids will include the PHV's ph_eval_at relids. This is a mirage though: we have or soon will remove these relids from the PHV's ph_eval_at, and therefore they no longer belong in qual clauses' clause_relids either. Remove that Assert in join_is_removable, and replace the similar one in remove_rel_from_query with code to remove the deleted relids from clause_relids. Per bug #17773 from Robins Tharakan. Discussion: https://postgr.es/m/17773-a592e6cedbc7bac5@postgresql.org
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
@@ -217,28 +217,36 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
 
 	/*
 	 * Similarly check that the inner rel isn't needed by any PlaceHolderVars
-	 * that will be used above the join.  We only need to fail if such a PHV
-	 * actually references some inner-rel attributes; but the correct check
-	 * for that is relatively expensive, so we first check against ph_eval_at,
-	 * which must mention the inner rel if the PHV uses any inner-rel attrs as
-	 * non-lateral references.  Note that if the PHV's syntactic scope is just
-	 * the inner rel, we can't drop the rel even if the PHV is variable-free.
+	 * that will be used above the join.  The PHV case is a little bit more
+	 * complicated, because PHVs may have been assigned a ph_eval_at location
+	 * that includes the innerrel, yet their contained expression might not
+	 * actually reference the innerrel (it could be just a constant, for
+	 * instance).  If such a PHV is due to be evaluated above the join then it
+	 * needn't prevent join removal.
 	 */
 	foreach(l, root->placeholder_list)
 	{
 		PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(l);
 
 		if (bms_overlap(phinfo->ph_lateral, innerrel->relids))
 			return false;		/* it references innerrel laterally */
-		if (bms_is_subset(phinfo->ph_needed, inputrelids))
-			continue;			/* PHV is not used above the join */
 		if (!bms_overlap(phinfo->ph_eval_at, innerrel->relids))
 			continue;			/* it definitely doesn't reference innerrel */
-		if (bms_is_subset(phinfo->ph_eval_at, innerrel->relids))
+		if (bms_is_subset(phinfo->ph_needed, inputrelids))
+			continue;			/* PHV is not used above the join */
+		if (!bms_is_member(sjinfo->ojrelid, phinfo->ph_eval_at))
+			return false;		/* it has to be evaluated below the join */
+
+		/*
+		 * We need to be sure there will still be a place to evaluate the PHV
+		 * if we remove the join, ie that ph_eval_at wouldn't become empty.
+		 */
+		if (!bms_overlap(sjinfo->min_lefthand, phinfo->ph_eval_at))
 			return false;		/* there isn't any other place to eval PHV */
+		/* Check contained expression last, since this is a bit expensive */
 		if (bms_overlap(pull_varnos(root, (Node *) phinfo->ph_var->phexpr),
 						innerrel->relids))
-			return false;		/* it does reference innerrel */
+			return false;		/* contained expression references innerrel */
 	}
 
 	/*
@@ -270,15 +278,7 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
 		 * be from WHERE, for example).
 		 */
 		if (RINFO_IS_PUSHED_DOWN(restrictinfo, joinrelids))
-		{
-			/*
-			 * If such a clause actually references the inner rel then join
-			 * removal has to be disallowed.  The previous attr_needed checks
-			 * should have caught this, though.
-			 */
-			Assert(!bms_is_member(innerrelid, restrictinfo->clause_relids));
 			continue;			/* ignore; not useful here */
-		}
 
 		/* Ignore if it's not a mergejoinable clause */
 		if (!restrictinfo->can_join ||
@@ -465,13 +465,21 @@ remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
 		 */
 		if (bms_is_member(ojrelid, rinfo->required_relids))
 		{
-			/* Recheck that qual doesn't actually reference the target rel */
-			Assert(!bms_is_member(relid, rinfo->clause_relids));
-
 			/*
-			 * The required_relids probably aren't shared with anything else,
+			 * There might be references to relid or ojrelid in the
+			 * clause_relids as a consequence of PHVs having ph_eval_at sets
+			 * that include those.  We already checked above that any such PHV
+			 * is safe, so we can just drop those references.
+			 *
+			 * The clause_relids probably aren't shared with anything else,
 			 * but let's copy them just to be sure.
 			 */
+			rinfo->clause_relids = bms_copy(rinfo->clause_relids);
+			rinfo->clause_relids = bms_del_member(rinfo->clause_relids,
+												  relid);
+			rinfo->clause_relids = bms_del_member(rinfo->clause_relids,
+												  ojrelid);
+			/* Likewise for required_relids */
 			rinfo->required_relids = bms_copy(rinfo->required_relids);
 			rinfo->required_relids = bms_del_member(rinfo->required_relids,
 													relid);
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
@@ -5213,6 +5213,26 @@ SELECT q2 FROM
          Index Cond: (id = int8_tbl.q2)
 (5 rows)
 
+-- join removal bug #17773: otherwise-removable PHV appears in a qual condition
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT q2 FROM
+  (SELECT q2, 'constant'::text AS x
+   FROM int8_tbl LEFT JOIN innertab ON q2 = id) ss
+  RIGHT JOIN int4_tbl ON NULL
+ WHERE x >= x;
+                      QUERY PLAN                      
+------------------------------------------------------
+ Nested Loop Left Join
+   Output: q2
+   Join Filter: NULL::boolean
+   Filter: (('constant'::text) >= ('constant'::text))
+   ->  Seq Scan on public.int4_tbl
+         Output: int4_tbl.f1
+   ->  Result
+         Output: q2, 'constant'::text
+         One-Time Filter: false
+(9 rows)
+
 rollback;
 -- another join removal bug: we must clean up correctly when removing a PHV
 begin;
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
@@ -1888,6 +1888,14 @@ SELECT q2 FROM
    FROM int8_tbl LEFT JOIN innertab ON q2 = id) ss
  WHERE COALESCE(dat1, 0) = q1;
 
+-- join removal bug #17773: otherwise-removable PHV appears in a qual condition
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT q2 FROM
+  (SELECT q2, 'constant'::text AS x
+   FROM int8_tbl LEFT JOIN innertab ON q2 = id) ss
+  RIGHT JOIN int4_tbl ON NULL
+ WHERE x >= x;
+
 rollback;
 
 -- another join removal bug: we must clean up correctly when removing a PHV
