remove_rel_from_query() must clean up PlaceHolderVar.phrels fields.

While we got away with this sloppiness before, it's not okay now
that fee7b77b9 caused build_joinrel_tlist() to make use of phrels.
Per report from Robins Tharakan.

Richard Guo (some cosmetic tweaks by me)

Discussion: https://postgr.es/m/CAMbWs4_ngw9sKxpTE8hqk=-ooVX_CQP3DarA4HzkRMz_JKpTrA@mail.gmail.com

3 files changed, 33 insertions, 7 deletions

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 5bcf95cef3e..283539e53c4 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -316,12 +316,8 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
  * Remove the target relid from the planner's data structures, having
  * determined that there is no need to include it in the query.
  *
- * We are not terribly thorough here.  We must make sure that the rel is
- * no longer treated as a baserel, and that attributes of other baserels
- * are no longer marked as being needed at joins involving this rel.
- * Also, join quals involving the rel have to be removed from the joininfo
- * lists, but only if they belong to the outer join identified by ojrelid
- * and joinrelids.
+ * We are not terribly thorough here.  We only bother to update parts of
+ * the planner's data structures that will actually be consulted later.
  */
 static void
 remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
@@ -429,11 +425,18 @@ remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
 		}
 		else
 		{
+			PlaceHolderVar *phv = phinfo->ph_var;
+
 			phinfo->ph_eval_at = bms_del_member(phinfo->ph_eval_at, relid);
 			phinfo->ph_eval_at = bms_del_member(phinfo->ph_eval_at, ojrelid);
-			Assert(!bms_is_empty(phinfo->ph_eval_at));
+			Assert(!bms_is_empty(phinfo->ph_eval_at));	/* checked previously */
 			phinfo->ph_needed = bms_del_member(phinfo->ph_needed, relid);
 			phinfo->ph_needed = bms_del_member(phinfo->ph_needed, ojrelid);
+			/* ph_needed might or might not become empty */
+			phv->phrels = bms_del_member(phv->phrels, relid);
+			phv->phrels = bms_del_member(phv->phrels, ojrelid);
+			Assert(!bms_is_empty(phv->phrels));
+			Assert(phv->phnullingrels == NULL); /* no need to adjust */
 		}
 	}
 
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 18b5e8f750b..75f03fcf30c 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -5052,6 +5052,22 @@ from int4_tbl as t1
          One-Time Filter: false
 (5 rows)
 
+-- per further discussion of bug #17781
+explain (costs off)
+select ss1.x
+from (select f1/2 as x from int4_tbl i4 left join a on a.id = i4.f1) ss1
+     right join int8_tbl i8 on true
+where current_user is not null;  -- this is to add a Result node
+                  QUERY PLAN                   
+-----------------------------------------------
+ Result
+   One-Time Filter: (CURRENT_USER IS NOT NULL)
+   ->  Nested Loop Left Join
+         ->  Seq Scan on int8_tbl i8
+         ->  Materialize
+               ->  Seq Scan on int4_tbl i4
+(6 rows)
+
 -- check that join removal works for a left join when joining a subquery
 -- that is guaranteed to be unique by its GROUP BY clause
 explain (costs off)
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 7806c910b4d..2e764cdd51d 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1813,6 +1813,13 @@ from int4_tbl as t1
                on t5.q1 = t7.q2)
     on false;
 
+-- per further discussion of bug #17781
+explain (costs off)
+select ss1.x
+from (select f1/2 as x from int4_tbl i4 left join a on a.id = i4.f1) ss1
+     right join int8_tbl i8 on true
+where current_user is not null;  -- this is to add a Result node
+
 -- check that join removal works for a left join when joining a subquery
 -- that is guaranteed to be unique by its GROUP BY clause
 explain (costs off)