Fix up join removal's interaction with PlaceHolderVars.

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 8538519db 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
index 1ca554a643002bed68cc4272561fad298ff5b3a8..5bcf95cef3e0e85e31be48a73830598167f44faa 100644 (file)
--- 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 037c7d0d566315c60bf645bb4a21608d0c5870dc..07f5aad5ea4b190bcaecaab42691aa1e6b46f1f4 100644 (file)
--- 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 1f2b7f62f0fb9eb525774f1c7ebe4c473d0528dd..7157b5ccccac57c43d2b2d3a752f363641e112c9 100644 (file)
--- 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