From 36356ac53b86a203870767b9262c6ed5912bf794 Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Tue, 14 Jul 2026 14:42:08 +0900 Subject: [PATCH v2] 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 Reviewed-by: Tender Wang Reviewed-by: Richard Guo Discussion: https://postgr.es/m/27a44087-3d65-473e-8d88-7c12228e0d7e@malkut.net Backpatch-through: 18 --- src/backend/optimizer/plan/analyzejoins.c | 78 +++++++++++++++++++++-- src/test/regress/expected/join.out | 12 ++++ src/test/regress/sql/join.sql | 7 ++ 3 files changed, 92 insertions(+), 5 deletions(-) diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c index f109af25d72..881950e5264 100644 --- 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 ed946abed7f..83bd5649d5c 100644 --- 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 78f7b4f544d..32d4a5a677e 100644 --- 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); -- 2.39.5 (Apple Git-154)