Fix old oversight in join removal logic.

Commit 9e7e29c75ad441450f9b8287bd51c13521641e3b introduced an Assert that
join removal didn't reduce the eval_at set of any PlaceHolderVar to empty.
At first glance it looks like join_is_removable ensures that's true --- but
actually, the loop in join_is_removable skips PlaceHolderVars that are not
referenced above the join due to be removed.  So, if we don't want any
empty eval_at sets, the right thing to do is to delete any now-unreferenced
PlaceHolderVars from the data structure entirely.

Per fuzz testing by Andreas Seltenreich.  Back-patch to 9.3 where the
aforesaid Assert was added.

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 129fc3dfae6cd2937a8e0a21003f314d8d034853..6b967df37798f2838fe13dea2e7a3c9b266ab8c0 100644 (file)
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -382,16 +382,24 @@ remove_rel_from_query(PlannerInfo *root, int relid, Relids joinrelids)
    }
 
    /*
-    * Likewise remove references from PlaceHolderVar data structures.
+    * Likewise remove references from PlaceHolderVar data structures,
+    * removing any no-longer-needed placeholders entirely.
     */
-   foreach(l, root->placeholder_list)
+   for (l = list_head(root->placeholder_list); l != NULL; l = nextl)
    {
        PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(l);
 
-       phinfo->ph_eval_at = bms_del_member(phinfo->ph_eval_at, relid);
-       Assert(!bms_is_empty(phinfo->ph_eval_at));
-       Assert(!bms_is_member(relid, phinfo->ph_lateral));
-       phinfo->ph_needed = bms_del_member(phinfo->ph_needed, relid);
+       nextl = lnext(l);
+       if (bms_is_subset(phinfo->ph_needed, joinrelids))
+           root->placeholder_list = list_delete_ptr(root->placeholder_list,
+                                                    phinfo);
+       else
+       {
+           phinfo->ph_eval_at = bms_del_member(phinfo->ph_eval_at, relid);
+           Assert(!bms_is_empty(phinfo->ph_eval_at));
+           Assert(!bms_is_member(relid, phinfo->ph_lateral));
+           phinfo->ph_needed = bms_del_member(phinfo->ph_needed, relid);
+       }
    }
 
    /*

diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 6c3d8fd7a04fab4c47415a6af9f176e5fe39bd74..68e688b9d3c9ddc79069f3d958d0066616f942dc 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -3743,6 +3743,22 @@ SELECT * FROM
  1 | 4567890123456789 | -4567890123456789 | 4567890123456789
 (5 rows)
 
+rollback;
+-- another join removal bug: we must clean up correctly when removing a PHV
+begin;
+create temp table uniquetbl (f1 text unique);
+explain (costs off)
+select t1.* from
+  uniquetbl as t1
+  left join (select *, '***'::text as d1 from uniquetbl) t2
+  on t1.f1 = t2.f1
+  left join uniquetbl t3
+  on t2.d1 = t3.f1;
+        QUERY PLAN        
+--------------------------
+ Seq Scan on uniquetbl t1
+(1 row)
+
 rollback;
 -- bug #8444: we've historically allowed duplicate aliases within aliased JOINs
 select * from

diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 992e673c12e299ac837eede8733f44090e22e381..402ba61eac794212e788e54640f94bf26b69ca86 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1196,6 +1196,21 @@ SELECT * FROM
 
 rollback;
 
+-- another join removal bug: we must clean up correctly when removing a PHV
+begin;
+
+create temp table uniquetbl (f1 text unique);
+
+explain (costs off)
+select t1.* from
+  uniquetbl as t1
+  left join (select *, '***'::text as d1 from uniquetbl) t2
+  on t1.f1 = t2.f1
+  left join uniquetbl t3
+  on t2.d1 = t3.f1;
+
+rollback;
+
 -- bug #8444: we've historically allowed duplicate aliases within aliased JOINs
 
 select * from