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 5557600988d84d9573c67d018c96389aa724151e..4d19204da0a2036ba7863d8522a3954eee5653ab 100644 (file)
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -75,6 +75,8 @@ static void remove_rel_from_restrictinfo(RestrictInfo *rinfo,
 static void remove_rel_from_eclass(PlannerInfo *root, EquivalenceClass *ec,
                                                                   SpecialJoinInfo *sjinfo,
                                                                   int relid, int subst);
+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);
@@ -347,6 +349,7 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
        int                     relid = rel->relid;
        Index           rti;
        ListCell   *l;
+       Bitmapset  *seen_serials = NULL;
 
        /*
         * Update all_baserels and related relid sets.
@@ -522,9 +525,9 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
         * lateral_vars lists.  (We already did this above for ph_needed.)
         *
         * 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++)
        {
@@ -554,10 +557,27 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
                if (sjinfo != NULL && rti != relid && root->glob->lastPHId != 0)
                {
                        foreach_node(RestrictInfo, rinfo, otherrel->baserestrictinfo)
+                               remove_rel_from_restrictinfo_phvs(rinfo, relid, sjinfo->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,
-                                                                                sjinfo->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, sjinfo->ojrelid);
                        }
                }
        }
@@ -847,6 +867,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 e9a5ecb1581f810b0480f17f4af2534e399dd02c..86078cbf27d505c6bb79f8c618a67b45b614ca8a 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -6394,6 +6394,17 @@ group by ();
  Result
 (1 row)
 
+-- 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
+(1 row)
+
 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 d0f36bec25c98827a61a33baa533a043d3ac3f82..3aaa882d6687357aff85d55a6a0b6d2733dc6907 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2354,6 +2354,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);