Fix join removal logic to clean up sub-RestrictInfos of OR clauses.

analyzejoins.c took care to clean out removed relids from the
clause_relids and required_relids of RestrictInfos associated with
the doomed rel ... but it paid no attention to the fact that if such a
RestrictInfo contains an OR clause, there will be sub-RestrictInfos
containing similar fields.

I'm more than a bit surprised that this oversight hasn't caused
visible problems before.  In any case, it's certainly broken now,
so add logic to clean out the sub-RestrictInfos recursively.
We might need to back-patch this someday.

Per bug #17786 from Robins Tharakan.

Discussion: https://postgr.es/m/17786-f1ea7fbdab97daec@postgresql.org

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 283539e53c404eb5748de0e244c98ec6f35739aa..7fd11df9afb2297f6dc19b7f67aca9be26eec3ed 100644 (file)
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -29,6 +29,7 @@
 #include "optimizer/pathnode.h"
 #include "optimizer/paths.h"
 #include "optimizer/planmain.h"
+#include "optimizer/restrictinfo.h"
 #include "optimizer/tlist.h"
 #include "utils/lsyscache.h"
 
@@ -36,6 +37,8 @@
 static bool join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo);
 static void remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
                                  Relids joinrelids);
+static void remove_rel_from_restrictinfo(RestrictInfo *rinfo,
+                                        int relid, int ojrelid);
 static List *remove_rel_from_joinlist(List *joinlist, int relid, int *nremoved);
 static bool rel_supports_distinctness(PlannerInfo *root, RelOptInfo *rel);
 static bool rel_is_distinct_for(PlannerInfo *root, RelOptInfo *rel,
@@ -470,24 +473,12 @@ remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
        {
            /*
             * There might be references to relid or ojrelid in the
-            * clause_relids as a consequence of PHVs having ph_eval_at sets
+            * RestrictInfo, 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);
-           rinfo->required_relids = bms_del_member(rinfo->required_relids,
-                                                   ojrelid);
+           remove_rel_from_restrictinfo(rinfo, relid, ojrelid);
+           /* Now throw it back into the joininfo lists */
            distribute_restrictinfo_to_rels(root, rinfo);
        }
    }
@@ -498,6 +489,62 @@ remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
     */
 }
 
+/*
+ * Remove any references to relid or ojrelid from the RestrictInfo.
+ *
+ * We only bother to clean out bits in clause_relids and required_relids,
+ * not nullingrel bits in contained Vars and PHVs.  (This might have to be
+ * improved sometime.)  However, if the RestrictInfo contains an OR clause
+ * we have to also clean up the sub-clauses.
+ */
+static void
+remove_rel_from_restrictinfo(RestrictInfo *rinfo, int relid, int ojrelid)
+{
+   /*
+    * 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);
+   rinfo->required_relids = bms_del_member(rinfo->required_relids, ojrelid);
+
+   /* If it's an OR, recurse to clean up sub-clauses */
+   if (restriction_is_or_clause(rinfo))
+   {
+       ListCell   *lc;
+
+       Assert(is_orclause(rinfo->orclause));
+       foreach(lc, ((BoolExpr *) rinfo->orclause)->args)
+       {
+           Node       *orarg = (Node *) lfirst(lc);
+
+           /* OR arguments should be ANDs or sub-RestrictInfos */
+           if (is_andclause(orarg))
+           {
+               List       *andargs = ((BoolExpr *) orarg)->args;
+               ListCell   *lc2;
+
+               foreach(lc2, andargs)
+               {
+                   RestrictInfo *rinfo2 = lfirst_node(RestrictInfo, lc2);
+
+                   remove_rel_from_restrictinfo(rinfo2, relid, ojrelid);
+               }
+           }
+           else
+           {
+               RestrictInfo *rinfo2 = castNode(RestrictInfo, orarg);
+
+               remove_rel_from_restrictinfo(rinfo2, relid, ojrelid);
+           }
+       }
+   }
+}
+
 /*
  * Remove any occurrences of the target relid from a joinlist structure.
  *

diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 98d611412dfb562205d1fb26cf845bc0931a49a3..2f1f8b8dbe6730e1a1255cbedd0d1afc19a084c0 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -5344,6 +5344,26 @@ SELECT q2 FROM
          One-Time Filter: false
 (9 rows)
 
+-- join removal bug #17786: check that OR conditions are cleaned up
+EXPLAIN (COSTS OFF)
+SELECT f1, x
+FROM int4_tbl
+     JOIN ((SELECT 42 AS x FROM int8_tbl LEFT JOIN innertab ON q1 = id) AS ss1
+           RIGHT JOIN tenk1 ON NULL)
+        ON tenk1.unique1 = ss1.x OR tenk1.unique2 = ss1.x;
+                                QUERY PLAN                                
+--------------------------------------------------------------------------
+ Nested Loop
+   ->  Seq Scan on int4_tbl
+   ->  Materialize
+         ->  Nested Loop Left Join
+               Join Filter: NULL::boolean
+               Filter: ((tenk1.unique1 = (42)) OR (tenk1.unique2 = (42)))
+               ->  Seq Scan on tenk1
+               ->  Result
+                     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 e50a769606938719791eeaf4e6aaaf416bc1b4bd..400c16958f31465576e5004f9604af852ec120e4 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1955,6 +1955,14 @@ SELECT q2 FROM
   RIGHT JOIN int4_tbl ON NULL
  WHERE x >= x;
 
+-- join removal bug #17786: check that OR conditions are cleaned up
+EXPLAIN (COSTS OFF)
+SELECT f1, x
+FROM int4_tbl
+     JOIN ((SELECT 42 AS x FROM int8_tbl LEFT JOIN innertab ON q1 = id) AS ss1
+           RIGHT JOIN tenk1 ON NULL)
+        ON tenk1.unique1 = ss1.x OR tenk1.unique2 = ss1.x;
+
 rollback;
 
 -- another join removal bug: we must clean up correctly when removing a PHV