From 4e0512741c757b29af1e4b69579aea1dcc9ffba0 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 26 Jul 2026 12:38:13 -0400 Subject: [PATCH v4 4/5] Ensure we hoist modified quals to the right join level after SJE. The v2 patchset in this series failed to consider whether a modified qual was actually appropriate for its original position in the join tree. It might not be, in which case we'd better move it up. Reported-by: Thom Brown Author: Tom Lane Discussion: https://postgr.es/m/1186816.1784573544@sss.pgh.pa.us --- src/backend/optimizer/plan/analyzejoins.c | 103 +++++++++++++++++++--- src/test/regress/expected/join.out | 58 ++++++++++++ src/test/regress/sql/join.sql | 18 ++++ 3 files changed, 169 insertions(+), 10 deletions(-) diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c index a038b898bdb..8fe382c4287 100644 --- a/src/backend/optimizer/plan/analyzejoins.c +++ b/src/backend/optimizer/plan/analyzejoins.c @@ -87,8 +87,8 @@ static bool is_innerrel_unique_for(PlannerInfo *root, static Node *remove_rel_from_jointree(Node *jtnode, int relid, Node **orphan_quals, int *nremoved); static Node *merge_quals(Node *quals1, Node *quals2); -static void fixup_selfjoin_jointree(PlannerInfo *root, Node *jtnode, - int relid); +static void fixup_selfjoin_jointree(PlannerInfo *root, Node *jtnode, int relid, + Node **hoist_quals, bool *found_relid); static List *fixup_selfjoin_quals(PlannerInfo *root, List *quals, int relid); static Node *replace_selfjoin_qual(Node *qual); static int self_join_candidates_cmp(const void *a, const void *b); @@ -1201,7 +1201,9 @@ is_innerrel_unique_for(PlannerInfo *root, * jointree and then pointing everything that referenced it at the relation we * are keeping. All the conditions that were attached to the removed relation * thereby become conditions on the remaining one, which is what we want: - * we've proven that the two relations select the same rows. + * we've proven that the two relations select the same rows. Note that + * this change requires us to hoist those conditions up to someplace + * syntactically enclosing toKeep. * * kmark and rmark are the PlanRowMarks (if any) for the kept and removed * relations. We could re-locate those, but the caller already found them. @@ -1213,6 +1215,8 @@ remove_self_join_rel(PlannerInfo *root, { Node *orphan_quals = NULL; int nremoved = 0; + Node *hoist_quals = NULL; + bool found_relid = false; Assert(toKeep->relid > 0); Assert(toRemove->relid > 0); @@ -1251,7 +1255,11 @@ remove_self_join_rel(PlannerInfo *root, /* Clean up the quals that the substitution has messed with */ fixup_selfjoin_jointree(root, (Node *) root->parse->jointree, - toKeep->relid); + toKeep->relid, + &hoist_quals, &found_relid); + /* We shouldn't have any leftover quals, and we must have found toKeep */ + Assert(hoist_quals == NULL); + Assert(found_relid); /* * If the removed relation has a row mark, transfer it to the remaining @@ -1400,24 +1408,69 @@ merge_quals(Node *quals1, Node *quals2) * Clean up the query's jointree quals after self-join elimination has * merged one relation into another. (relid is the kept relation.) * - * See fixup_selfjoin_quals() for what needs fixing. + * See fixup_selfjoin_quals() for what needs fixing locally to each qual list. + * In addition, we need to check quals to see if they refer to relid, and if + * so make sure they get hoisted to someplace syntactically above relid. + * Do that using a "hoist_quals" in/out parameter similar to "orphan_quals" + * in remove_rel_from_jointree. (We can't readily merge these concerns into + * a single pass, since remove_rel_from_jointree must run before we relabel + * the removed rel's Vars.) In addition, *found_relid is set true if + * the subtree rooted at jtnode is found to contain relid's RangeTblRef, + * so that we can tell when to stop hoisting quals. + * If a qual gets hoisted up, we apply fixup_selfjoin_quals() to it only + * after it reaches its final level. This rule improves the odds of + * detecting duplicate quals. */ static void -fixup_selfjoin_jointree(PlannerInfo *root, Node *jtnode, int relid) +fixup_selfjoin_jointree(PlannerInfo *root, Node *jtnode, int relid, + Node **hoist_quals, bool *found_relid) { if (jtnode == NULL) return; if (IsA(jtnode, RangeTblRef)) { - /* nothing to do here */ + RangeTblRef *rtr = (RangeTblRef *) jtnode; + + if (rtr->rtindex == relid) + { + Assert(!*found_relid); + *found_relid = true; + } } else if (IsA(jtnode, FromExpr)) { FromExpr *f = (FromExpr *) jtnode; + Node *sub_hoist_quals = NULL; + bool sub_found_relid = false; ListCell *l; foreach(l, f->fromlist) - fixup_selfjoin_jointree(root, (Node *) lfirst(l), relid); + fixup_selfjoin_jointree(root, (Node *) lfirst(l), relid, + &sub_hoist_quals, &sub_found_relid); + if (sub_found_relid) + { + /* This FromExpr covers relid, so OK to stop hoisting quals here */ + f->quals = merge_quals(sub_hoist_quals, f->quals); + Assert(!*found_relid); + *found_relid = true; + } + else + { + /* We might need to hoist some of our own quals too */ + List *hoistable = NIL; + List *keepable = NIL; + + foreach_ptr(Node, qual, castNode(List, f->quals)) + { + if (bms_is_member(relid, pull_varnos(root, qual))) + hoistable = lappend(hoistable, qual); + else + keepable = lappend(keepable, qual); + } + f->quals = (Node *) keepable; + sub_hoist_quals = merge_quals(sub_hoist_quals, (Node *) hoistable); + *hoist_quals = merge_quals(sub_hoist_quals, *hoist_quals); + } f->quals = (Node *) fixup_selfjoin_quals(root, castNode(List, f->quals), relid); @@ -1425,9 +1478,39 @@ fixup_selfjoin_jointree(PlannerInfo *root, Node *jtnode, int relid) else if (IsA(jtnode, JoinExpr)) { JoinExpr *j = (JoinExpr *) jtnode; + Node *sub_hoist_quals = NULL; + bool sub_found_relid = false; + + fixup_selfjoin_jointree(root, j->larg, relid, + &sub_hoist_quals, &sub_found_relid); + fixup_selfjoin_jointree(root, j->rarg, relid, + &sub_hoist_quals, &sub_found_relid); + if (sub_found_relid) + { + /* This JoinExpr covers relid, so OK to stop hoisting quals here */ + j->quals = merge_quals(sub_hoist_quals, j->quals); + Assert(!*found_relid); + *found_relid = true; + } + else + { + /* We might need to hoist some of our own quals too */ + List *hoistable = NIL; + List *keepable = NIL; - fixup_selfjoin_jointree(root, j->larg, relid); - fixup_selfjoin_jointree(root, j->rarg, relid); + foreach_ptr(Node, qual, castNode(List, j->quals)) + { + if (bms_is_member(relid, pull_varnos(root, qual))) + hoistable = lappend(hoistable, qual); + else + keepable = lappend(keepable, qual); + } + j->quals = (Node *) keepable; + sub_hoist_quals = merge_quals(sub_hoist_quals, (Node *) hoistable); + /* We should never need to hoist quals above an outer join */ + Assert(sub_hoist_quals == NULL || j->jointype == JOIN_INNER); + *hoist_quals = merge_quals(sub_hoist_quals, *hoist_quals); + } j->quals = (Node *) fixup_selfjoin_quals(root, castNode(List, j->quals), relid); diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 254b203aeba..57edb38a237 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -7478,6 +7478,64 @@ select t1.a from sj t1 where t1.b in ( Filter: (t3.c IS NOT NULL) (10 rows) +-- Check that quals get hoisted to the appropriate join level after SJE removal +explain (verbose, costs off) +select a2.a +from sj b1 + join sj a1 on b1.b = a1.a + join sj a2 on a2.a = a1.a and a2.b = a1.b; + QUERY PLAN +------------------------------------------------------------- + Nested Loop + Output: a2.a + Join Filter: (b1.b = a2.a) + -> Seq Scan on public.sj a2 + Output: a2.a, a2.b, a2.c + Filter: ((a2.a IS NOT NULL) AND (a2.b IS NOT NULL)) + -> Seq Scan on public.sj b1 + Output: b1.a, b1.b, b1.c +(8 rows) + +-- Same, when a semijoin removal happens first +explain (verbose, costs off) +select a1.a from sj b1 join sj a1 on a1.a = b1.b + where exists (select 1 from sj s where s.a = a1.a); + QUERY PLAN +----------------------------------------- + Nested Loop + Output: s.a + Inner Unique: true + Join Filter: (b1.b = s.a) + -> Seq Scan on public.sj b1 + Output: b1.a, b1.b, b1.c + -> Materialize + Output: s.a + -> Seq Scan on public.sj s + Output: s.a + Filter: (s.a IS NOT NULL) +(11 rows) + +-- A different case, where modified qual is on a lower join level +explain (verbose, costs off) +select a2.a +from ((sj b1 join sj a1 on true) join sj c1 on c1.b = a1.a) + join sj a2 on a2.a = a1.a and a2.b = a1.b; + QUERY PLAN +------------------------------------------------------------------- + Nested Loop + Output: a2.a + -> Nested Loop + Output: a2.a + Join Filter: (c1.b = a2.a) + -> Seq Scan on public.sj a2 + Output: a2.a, a2.b, a2.c + Filter: ((a2.a IS NOT NULL) AND (a2.b IS NOT NULL)) + -> Seq Scan on public.sj c1 + Output: c1.a, c1.b, c1.c + -> Seq Scan on public.sj b1 + Output: b1.a, b1.b, b1.c +(12 rows) + -- -- SJE corner case: uniqueness of an inner is [partially] derived from -- baserestrictinfo clauses. diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index afa96daecb5..f2291c67190 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2892,6 +2892,24 @@ explain (verbose, costs off) select t1.a from sj t1 where t1.b in ( select t2.b from sj t2 join sj t3 on t2.c=t3.c); +-- Check that quals get hoisted to the appropriate join level after SJE removal +explain (verbose, costs off) +select a2.a +from sj b1 + join sj a1 on b1.b = a1.a + join sj a2 on a2.a = a1.a and a2.b = a1.b; + +-- Same, when a semijoin removal happens first +explain (verbose, costs off) +select a1.a from sj b1 join sj a1 on a1.a = b1.b + where exists (select 1 from sj s where s.a = a1.a); + +-- A different case, where modified qual is on a lower join level +explain (verbose, costs off) +select a2.a +from ((sj b1 join sj a1 on true) join sj c1 on c1.b = a1.a) + join sj a2 on a2.a = a1.a and a2.b = a1.b; + -- -- SJE corner case: uniqueness of an inner is [partially] derived from -- baserestrictinfo clauses. -- 2.52.0