3 files changed, 58 insertions, 22 deletions

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 1ca554a6430..5bcf95cef3e 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -217,12 +217,12 @@ 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)
 	{
@@ -230,15 +230,23 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
 
 		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
index 037c7d0d566..07f5aad5ea4 100644
--- 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
index 1f2b7f62f0f..7157b5cccca 100644
--- 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