Strip removed-relation references from PlaceHolderVars at join removal

When left-join removal deletes a relation, remove_rel_from_query()
updates the relid sets attached to RestrictInfos and
EquivalenceMembers, and the canonical PlaceHolderVar held in each
PlaceHolderInfo, but it does not rewrite the PlaceHolderVars embedded
in clause and EquivalenceClass member expressions.  That has been
fine, because later processing consults those relid sets rather than
the embedded PlaceHolderVars.

However, such an expression may afterwards be translated for an
appendrel child and have its relids recomputed from scratch by
pull_varnos().  If the embedded PlaceHolderVar's phrels still mentions
the removed relation, pull_varnos() folds it back in, so the rebuilt
clause's relids reference a no-longer-existent relation.  That yields
a parameterized path keyed on the removed relation, tripping the
Assert on root->outer_join_rels in get_eclass_indexes_for_relids().

Fix by stripping the removed relids from the PlaceHolderVars in
surviving rels' baserestrictinfo and in EquivalenceClass member
expressions, keeping them consistent with the canonical
PlaceHolderVars.

This is only reachable on v18 and later, where
match_index_to_operand() began ignoring PlaceHolderVars; before that,
the wrapping PlaceHolderVar prevented the index match that exposes the
stale relids.

Reported-by: Alexander Kuzmenkov <akuzmenkov@tigerdata.com>
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://postgr.es/m/CALzhyqwryL2QywgO03VQr_237Sq3MEVgTTT2_A9G3nGT5-SRZg@mail.gmail.com
Backpatch-through: 18

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index b07cb731401db7e5372e5e9d57e8811b3a0885c3..f109af25d72c96bb48d401b6a9b33dd5539765c4 100644 (file)
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -62,8 +62,10 @@ static void remove_rel_from_query(PlannerInfo *root, int relid,
                                                                  Relids joinrelids);
 static void remove_rel_from_restrictinfo(RestrictInfo *rinfo,
                                                                                 int relid, int ojrelid);
-static void remove_rel_from_eclass(EquivalenceClass *ec,
+static void remove_rel_from_eclass(PlannerInfo *root, EquivalenceClass *ec,
                                                                   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);
 static bool rel_supports_distinctness(PlannerInfo *root, RelOptInfo *rel);
 static bool rel_is_distinct_for(PlannerInfo *root, RelOptInfo *rel,
@@ -618,9 +620,7 @@ remove_rel_from_query(PlannerInfo *root, int relid,
                {
                        EquivalenceClass *ec = (EquivalenceClass *) lfirst(l);
 
-                       if (bms_is_member(relid, ec->ec_relids) ||
-                               bms_is_member(ojrelid, ec->ec_relids))
-                               remove_rel_from_eclass(ec, relid, ojrelid);
+                       remove_rel_from_eclass(root, ec, relid, ojrelid);
                }
        }
 
@@ -642,6 +642,11 @@ remove_rel_from_query(PlannerInfo *root, int relid,
         * Additionally, if we are performing self-join elimination, we must
         * replace references to the removed relid with subst within the
         * 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.
         */
        for (rti = 1; rti < root->simple_rel_array_size; rti++)
        {
@@ -667,6 +672,15 @@ remove_rel_from_query(PlannerInfo *root, int relid,
                if (is_self_join)
                        ChangeVarNodesExtended((Node *) otherrel->lateral_vars, relid,
                                                                   subst, 0, replace_relid_callback);
+
+               if (is_outer_join && rti != relid && root->glob->lastPHId != 0)
+               {
+                       foreach_node(RestrictInfo, rinfo, otherrel->baserestrictinfo)
+                       {
+                               rinfo->clause = (Expr *)
+                                       remove_rel_from_phvs((Node *) rinfo->clause, relid, ojrelid);
+                       }
+               }
        }
 }
 
@@ -748,17 +762,39 @@ remove_rel_from_restrictinfo(RestrictInfo *rinfo, int relid, int ojrelid)
 /*
  * Remove any references to relid or ojrelid from the EquivalenceClass.
  *
- * Like remove_rel_from_restrictinfo, we don't worry about cleaning out
- * any nullingrel bits in contained Vars and PHVs.  (This might have to be
- * improved sometime.)  We do need to fix the EC and EM relid sets to ensure
- * that implied join equalities will be generated at the appropriate join
- * level(s).
+ * We fix the EC and EM relid sets to ensure that implied join equalities will
+ * be generated at the appropriate join level(s).  We also strip the removed
+ * rel from PlaceHolderVars embedded in member expressions; a member's
+ * em_relids reflects ph_eval_at rather than the PHV's phrels, so the latter
+ * can still mention the removed rel even when em_relids does not.  Like
+ * remove_rel_from_restrictinfo, we don't bother with nullingrel bits in
+ * contained plain Vars.
  */
 static void
-remove_rel_from_eclass(EquivalenceClass *ec, int relid, int ojrelid)
+remove_rel_from_eclass(PlannerInfo *root, EquivalenceClass *ec,
+                                          int relid, int ojrelid)
 {
        ListCell   *lc;
 
+       /*
+        * Strip the removed rel/join from PlaceHolderVars in member expressions.
+        * This is needed even when the EC's relids don't mention the removed rel.
+        * Plain Vars and Consts can't contain a PlaceHolderVar, so skip them.
+        */
+       if (root->glob->lastPHId != 0)
+       {
+               foreach_node(EquivalenceMember, em, ec->ec_members)
+               {
+                       if (!IsA(em->em_expr, Var) && !IsA(em->em_expr, Const))
+                               em->em_expr = (Expr *)
+                                       remove_rel_from_phvs((Node *) em->em_expr, relid, ojrelid);
+               }
+       }
+
+       if (!bms_is_member(relid, ec->ec_relids) &&
+               !bms_is_member(ojrelid, ec->ec_relids))
+               return;
+
        /* Fix up the EC's overall relids */
        ec->ec_relids = bms_del_member(ec->ec_relids, relid);
        ec->ec_relids = bms_del_member(ec->ec_relids, ojrelid);
@@ -808,6 +844,69 @@ remove_rel_from_eclass(EquivalenceClass *ec, int relid, int ojrelid)
        ec_clear_derived_clauses(ec);
 }
 
+/*
+ * Remove any references to the specified RT index(es) from the phrels (and
+ * phnullingrels) of every PlaceHolderVar in the given expression.
+ *
+ * remove_rel_from_query() fixes up the relid sets of RestrictInfos and
+ * EquivalenceMembers, but not the PlaceHolderVars embedded in their
+ * expressions.  That's normally fine, but such an expression may later be
+ * translated for an appendrel child and have its relids recomputed by
+ * pull_varnos().  A leftover removed relid in phrels would then make
+ * pull_varnos() reference a nonexistent rel, so we strip it here to match the
+ * canonical PlaceHolderVar.
+ */
+static Node *
+remove_rel_from_phvs(Node *node, int relid, int ojrelid)
+{
+       Relids          removable = bms_add_member(bms_make_singleton(relid), ojrelid);
+
+       return remove_rel_from_phvs_mutator(node, removable);
+}
+
+static Node *
+remove_rel_from_phvs_mutator(Node *node, Relids removable)
+{
+       if (node == NULL)
+               return NULL;
+       if (IsA(node, PlaceHolderVar))
+       {
+               PlaceHolderVar *phv = (PlaceHolderVar *) node;
+               Relids          newphrels;
+
+               /* Upper-level PlaceHolderVars should be long gone at this point */
+               Assert(phv->phlevelsup == 0);
+
+               /* Copy the PlaceHolderVar and mutate what's below ... */
+               phv = (PlaceHolderVar *)
+                       expression_tree_mutator(node,
+                                                                       remove_rel_from_phvs_mutator,
+                                                                       removable);
+
+               /*
+                * ... then strip the removed rels from its relid sets.
+                *
+                * If stripping would empty phrels, the PHV is evaluated only at the
+                * removed relation(s); it then belongs to an EquivalenceMember that
+                * the caller drops immediately afterwards.  Leave such a PHV
+                * untouched rather than build one with empty phrels, which the rest
+                * of the planner assumes never occurs.
+                */
+               newphrels = bms_difference(phv->phrels, removable);
+               if (!bms_is_empty(newphrels))
+               {
+                       phv->phrels = newphrels;
+                       phv->phnullingrels = bms_difference(phv->phnullingrels,
+                                                                                               removable);
+               }
+
+               return (Node *) phv;
+       }
+       return expression_tree_mutator(node,
+                                                                  remove_rel_from_phvs_mutator,
+                                                                  removable);
+}
+
 /*
  * 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 78bf022f7b4e3475ccabb6a4392b5b65f3670098..ed946abed7fa35e14c1924d58022d476b0204c52 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -6564,6 +6564,28 @@ select a.* from a left join parted_b pb on a.b_id = pb.id;
  Seq Scan on a
 (1 row)
 
+-- test that clauses that still embed PHVs are not referencing the removed
+-- relation when rebuilt for a partition of the kept relation
+explain (costs off)
+select 1 from (select t1.id from parted_b t1 left join parted_b t2 on t1.id = t2.id) s
+where s.id = 1 group by ();
+      QUERY PLAN       
+-----------------------
+ Result
+   Replaces: Aggregate
+(2 rows)
+
+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 = s.id
+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 fae19113cefba28a444f33c17f57cac11e4f4804..78f7b4f544d81b0ec2a7389c642e55b66a417084 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2420,6 +2420,18 @@ CREATE TEMP TABLE parted_b1 partition of parted_b for values from (0) to (10);
 explain (costs off)
 select a.* from a left join parted_b pb on a.b_id = pb.id;
 
+-- test that clauses that still embed PHVs are not referencing the removed
+-- relation when rebuilt for a partition of the kept relation
+explain (costs off)
+select 1 from (select t1.id from parted_b t1 left join parted_b t2 on t1.id = t2.id) s
+where s.id = 1 group by ();
+
+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 = s.id
+group by ();
+
 rollback;
 
 create temp table parent (k int primary key, pd int);