Strip removed-relation references from PHVs in join clauses

Commit 9a60f295b stripped the stale PlaceHolderVars left behind by
left-join removal from the surviving rels' baserestrictinfo and from
EquivalenceClass member expressions, but it overlooked join clauses.
A PlaceHolderVar embedded in a join clause can likewise retain the
removed rel and join in its phrels, since remove_rel_from_query()
fixes up the RestrictInfo's own relid sets but not the PHVs inside its
expression.

As before, this is normally harmless, because later processing
consults those relid sets rather than the embedded PHVs.  However, a
restriction clause derived from such an OR join clause inherits the
stale PlaceHolderVar, and when the derived clause is translated for an
appendrel child, pull_varnos() recomputes its relids and folds the
removed relation back in.  The rebuilt clause then references a
no-longer-existent relation, tripping an assertion during path
generation.

Fix by also stripping the removed relation from the PlaceHolderVars in
the surviving rels' join clauses, including the sub-clauses of any OR
clause.

Like 9a60f295b, this is only reachable on v18 and later, where
match_index_to_operand() began ignoring PlaceHolderVars.

Author: Arne Roland <arne.roland@malkut.net>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/27a44087-3d65-473e-8d88-7c12228e0d7e@malkut.net
Backpatch-through: 18

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index f109af25d72c96bb48d401b6a9b33dd5539765c4..881950e5264443b162dc01f86d9fb6271799cabf 100644 (file)
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -64,6 +64,8 @@ static void remove_rel_from_restrictinfo(RestrictInfo *rinfo,
                                                                                 int relid, int ojrelid);
 static void remove_rel_from_eclass(PlannerInfo *root, EquivalenceClass *ec,
                                                                   int relid, int ojrelid);
+static void remove_rel_from_restrictinfo_phvs(RestrictInfo *rinfo,
+                                                                                         int relid, int ojrelid);
 static Node *remove_rel_from_phvs(Node *node, int relid, int ojrelid);
 static Node *remove_rel_from_phvs_mutator(Node *node, Relids removable);
 static List *remove_rel_from_joinlist(List *joinlist, int relid, int *nremoved);
@@ -457,6 +459,7 @@ remove_rel_from_query(PlannerInfo *root, int relid,
        ListCell   *l;
        bool            is_outer_join = (sjinfo != NULL);
        bool            is_self_join = (!is_outer_join && subst > 0);
+       Bitmapset  *seen_serials = NULL;
 
        Assert(is_outer_join || is_self_join);
        Assert(!is_outer_join || ojrelid > 0);
@@ -644,9 +647,9 @@ remove_rel_from_query(PlannerInfo *root, int relid,
         * lateral_vars lists.
         *
         * Also, for left-join removal, we strip the removed rel and join from any
-        * PlaceHolderVar embedded in the surviving rels' restriction clauses (see
-        * remove_rel_from_phvs); we needn't bother with the rel being removed,
-        * nor when the query has no PlaceHolderVars.
+        * PlaceHolderVar embedded in the surviving rels' restriction clauses and
+        * join clauses; we needn't bother with the rel being removed, nor when
+        * the query has no PlaceHolderVars.
         */
        for (rti = 1; rti < root->simple_rel_array_size; rti++)
        {
@@ -676,9 +679,27 @@ remove_rel_from_query(PlannerInfo *root, int relid,
                if (is_outer_join && rti != relid && root->glob->lastPHId != 0)
                {
                        foreach_node(RestrictInfo, rinfo, otherrel->baserestrictinfo)
+                               remove_rel_from_restrictinfo_phvs(rinfo, relid, ojrelid);
+
+                       /*
+                        * Join clauses need the same treatment, but there's no value in
+                        * processing any join clause more than once.  So it's slightly
+                        * annoying that we have to find them via the per-base-relation
+                        * joininfo lists.  Avoid duplicate processing by tracking the
+                        * rinfo_serial numbers of join clauses we've already seen.  (This
+                        * doesn't work for is_clone clauses, so we must waste effort on
+                        * them.)
+                        */
+                       foreach_node(RestrictInfo, rinfo, otherrel->joininfo)
                        {
-                               rinfo->clause = (Expr *)
-                                       remove_rel_from_phvs((Node *) rinfo->clause, relid, ojrelid);
+                               if (!rinfo->is_clone)   /* else serial number is not unique */
+                               {
+                                       if (bms_is_member(rinfo->rinfo_serial, seen_serials))
+                                               continue;       /* saw it already */
+                                       seen_serials = bms_add_member(seen_serials,
+                                                                                                 rinfo->rinfo_serial);
+                               }
+                               remove_rel_from_restrictinfo_phvs(rinfo, relid, ojrelid);
                        }
                }
        }
@@ -844,6 +865,53 @@ remove_rel_from_eclass(PlannerInfo *root, EquivalenceClass *ec,
        ec_clear_derived_clauses(ec);
 }
 
+/*
+ * Remove any references to relid or ojrelid from the PlaceHolderVars embedded
+ * in a RestrictInfo's clause.
+ *
+ * If it's an OR clause, we must also fix up the orclause, which is a parallel
+ * representation built from its own sub-RestrictInfos.  We recurse into the
+ * sub-clauses for that, mirroring remove_rel_from_restrictinfo.
+ */
+static void
+remove_rel_from_restrictinfo_phvs(RestrictInfo *rinfo, int relid, int ojrelid)
+{
+       rinfo->clause = (Expr *)
+               remove_rel_from_phvs((Node *) rinfo->clause, relid, 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_phvs(rinfo2, relid, ojrelid);
+                               }
+                       }
+                       else
+                       {
+                               RestrictInfo *rinfo2 = castNode(RestrictInfo, orarg);
+
+                               remove_rel_from_restrictinfo_phvs(rinfo2, relid, ojrelid);
+                       }
+               }
+       }
+}
+
 /*
  * Remove any references to the specified RT index(es) from the phrels (and
  * phnullingrels) of every PlaceHolderVar in the given expression.

diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index ed946abed7fa35e14c1924d58022d476b0204c52..83bd5649d5c04504aa41d45fd48eb5eb83e63e59 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -6586,6 +6586,18 @@ group by ();
    Replaces: Aggregate
 (2 rows)
 
+-- likewise for a PHV embedded in an OR join clause
+explain (costs off)
+select 1 from parted_b t1
+  join (select t2.id from parted_b t2 left join parted_b t3 on t2.id = t3.id) s
+  on (t1.id = 1 and s.id = 2) or (t1.id = 3 and s.id = 4)
+group by ();
+      QUERY PLAN       
+-----------------------
+ Result
+   Replaces: Aggregate
+(2 rows)
+
 rollback;
 create temp table parent (k int primary key, pd int);
 create temp table child (k int unique, cd int);

diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 78f7b4f544d81b0ec2a7389c642e55b66a417084..32d4a5a677ea3c9179af12e6fb28eb693555bba4 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2432,6 +2432,13 @@ select 1 from parted_b t1
   on t1.id = s.id
 group by ();
 
+-- likewise for a PHV embedded in an OR join clause
+explain (costs off)
+select 1 from parted_b t1
+  join (select t2.id from parted_b t2 left join parted_b t3 on t2.id = t3.id) s
+  on (t1.id = 1 and s.id = 2) or (t1.id = 3 and s.id = 4)
+group by ();
+
 rollback;
 
 create temp table parent (k int primary key, pd int);