From 79df439cd98f2e408ff54e760b3d1cdc68e4c8c8 Mon Sep 17 00:00:00 2001 From: Matheus Alcantara Date: Thu, 16 Jul 2026 19:05:55 -0300 Subject: [PATCH v2] Fix wrong results from remove_useless_result_rtes with nested PHVs. find_dependent_phvs() and find_dependent_phvs_in_jointree() decide whether a PlaceHolderVar still needs the RTE_RESULT rel we're about to remove by comparing the PHV's phrels to a singleton set containing that rel's RT index. But phrels is documented to hold "base+OJ relids syntactically within" the PHV's expression, so it can contain outer-join RT indexes as well as base-relation ones. The comparison was done against the raw phrels, so a PHV whose phrels also included some enclosing outer join's RT index would never compare equal to the singleton, even though the PHV genuinely still depended on the RTE_RESULT rel. This let remove_useless_results_recurse conclude it was safe to discard the enclosing left join along with the RTE_RESULT, silently losing that join's null-extension semantics. A PHV-wrapped constant that should have gone to NULL for unmatched rows was then emitted unconditionally for every row. This can be seen with two nested commutable left joins where the lower join's RHS reduces to a single-row RTE_RESULT: the PHV built for the lower join's output picks up the outer join's RT index in its phrels alongside the two RTE_RESULT relids, which broke the exact-match test against either RTE_RESULT individually. Fix this by masking phrels down to base relids before comparing, since only base relids indicate a real evaluation-site dependency; the presence of an OJ relid doesn't. remove_useless_result_rtes now precomputes the set of base (non-join) RT indexes once and threads it through the recursion and into both helper functions, which intersect it with each candidate PHV's phrels before testing equality. Author: Matheus Alcantara Reported-by: leis@in.tum.de Discussion: https://www.postgresql.org/message-id/19553-4561747f93f368a7@postgresql.org --- src/backend/optimizer/prep/prepjointree.c | 82 ++++++++++++++++++----- src/test/regress/expected/join.out | 28 ++++++++ src/test/regress/sql/join.sql | 12 ++++ 3 files changed, 106 insertions(+), 16 deletions(-) diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index ca5ca8bfe22..e9098322182 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -167,12 +167,15 @@ static bool has_notnull_forced_var(PlannerInfo *root, List *forced_null_vars, reduce_outer_joins_pass1_state *right_state); static Node *remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, Node **parent_quals, - Relids *dropped_outer_joins); + Relids *dropped_outer_joins, + Relids baserels); static int get_result_relid(PlannerInfo *root, Node *jtnode); static void remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc); -static bool find_dependent_phvs(PlannerInfo *root, int varno); +static bool find_dependent_phvs(PlannerInfo *root, int varno, + Relids baserels); static bool find_dependent_phvs_in_jointree(PlannerInfo *root, - Node *node, int varno); + Node *node, int varno, + Relids baserels); static void substitute_phv_relids(Node *node, int varno, Relids subrelids); static void fix_append_rel_relids(PlannerInfo *root, int varno, @@ -3887,8 +3890,24 @@ void remove_useless_result_rtes(PlannerInfo *root) { Relids dropped_outer_joins = NULL; + Relids baserels = NULL; + int i; ListCell *cell; + /* + * Precompute the set of base (i.e., non-join) RT indexes in the query's + * rangetable. We'll need this below to filter out OJ relids when + * checking whether a PlaceHolderVar's phrels indicates a genuine + * dependency on some other relation, since phrels can contain OJ relids + * as well as base relids, and the presence of an OJ relid there doesn't + * mean the PHV must be evaluated at some other place. + */ + for (i = 1; i <= list_length(root->parse->rtable); i++) + { + if (rt_fetch(i, root->parse->rtable)->rtekind != RTE_JOIN) + baserels = bms_add_member(baserels, i); + } + /* Top level of jointree must always be a FromExpr */ Assert(IsA(root->parse->jointree, FromExpr)); /* Recurse ... */ @@ -3896,7 +3915,8 @@ remove_useless_result_rtes(PlannerInfo *root) remove_useless_results_recurse(root, (Node *) root->parse->jointree, NULL, - &dropped_outer_joins); + &dropped_outer_joins, + baserels); /* We should still have a FromExpr */ Assert(IsA(root->parse->jointree, FromExpr)); @@ -3955,11 +3975,14 @@ remove_useless_result_rtes(PlannerInfo *root) * the parent's quals list; otherwise, pass NULL for parent_quals. * (Note that in some cases, parent_quals points to the quals of a parent * more than one level up in the tree.) + * + * baserels is the set of base (non-join) RT indexes in the whole query. */ static Node * remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, Node **parent_quals, - Relids *dropped_outer_joins) + Relids *dropped_outer_joins, + Relids baserels) { Assert(jtnode != NULL); if (IsA(jtnode, RangeTblRef)) @@ -3989,7 +4012,8 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, /* Recursively transform child, allowing it to push up quals ... */ child = remove_useless_results_recurse(root, child, &f->quals, - dropped_outer_joins); + dropped_outer_joins, + baserels); /* ... and stick it back into the tree */ lfirst(cell) = child; @@ -4001,7 +4025,8 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, */ if (list_length(f->fromlist) > 1 && (varno = get_result_relid(root, child)) != 0 && - !find_dependent_phvs_in_jointree(root, (Node *) f, varno)) + !find_dependent_phvs_in_jointree(root, (Node *) f, varno, + baserels)) { f->fromlist = foreach_delete_current(f->fromlist, cell); result_relids = bms_add_member(result_relids, varno); @@ -4074,12 +4099,14 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, &j->quals : (j->jointype == JOIN_LEFT) ? parent_quals : NULL, - dropped_outer_joins); + dropped_outer_joins, + baserels); j->rarg = remove_useless_results_recurse(root, j->rarg, (j->jointype == JOIN_INNER || j->jointype == JOIN_LEFT) ? &j->quals : NULL, - dropped_outer_joins); + dropped_outer_joins, + baserels); /* Apply join-type-specific optimization rules */ switch (j->jointype) @@ -4103,7 +4130,8 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, * allowed to have such refs. */ if ((varno = get_result_relid(root, j->larg)) != 0 && - !find_dependent_phvs_in_jointree(root, j->rarg, varno)) + !find_dependent_phvs_in_jointree(root, j->rarg, varno, + baserels)) { remove_result_refs(root, varno, j->rarg); if (j->quals != NULL && parent_quals == NULL) @@ -4158,7 +4186,7 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, */ if ((varno = get_result_relid(root, j->rarg)) != 0 && (j->quals == NULL || - !find_dependent_phvs(root, varno))) + !find_dependent_phvs(root, varno, baserels))) { remove_result_refs(root, varno, j->larg); *dropped_outer_joins = bms_add_member(*dropped_outer_joins, @@ -4283,6 +4311,17 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc) * find_dependent_phvs - are there any PlaceHolderVars whose relids are * exactly the given varno? * + * "relids" means the PHV's phrels with any outer-join relids masked + * off by intersecting with the caller-supplied "baserels". phrels can + * contain OJ relids as well as base relids (it's the set of base+OJ relids + * syntactically within the PHV's expression), but the presence of an OJ + * relid there doesn't create any additional place where the PHV must be + * evaluated; it's only base relids that pin down the evaluation location. + * If we compared phrels as-is, we could wrongly conclude that a PHV isn't + * dependent on a RTE_RESULT rel we're about to remove, just because the + * PHV's phrels also happens to include some OJ that sits between the PHV + * and the RTE_RESULT syntactically. + * * find_dependent_phvs should be used when we want to see if there are * any such PHVs anywhere in the Query. Another use-case is to see if * a subtree of the join tree contains such PHVs; but for that, we have @@ -4293,6 +4332,7 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc) typedef struct { Relids relids; + Relids baserels; /* set of base (non-OJ) RT indexes in query */ int sublevels_up; } find_dependent_phvs_context; @@ -4306,9 +4346,16 @@ find_dependent_phvs_walker(Node *node, { PlaceHolderVar *phv = (PlaceHolderVar *) node; - if (phv->phlevelsup == context->sublevels_up && - bms_equal(context->relids, phv->phrels)) - return true; + if (phv->phlevelsup == context->sublevels_up) + { + Relids phbaserels = bms_intersect(phv->phrels, + context->baserels); + bool match = bms_equal(context->relids, phbaserels); + + bms_free(phbaserels); + if (match) + return true; + } /* fall through to examine children */ } if (IsA(node, Query)) @@ -4332,7 +4379,7 @@ find_dependent_phvs_walker(Node *node, } static bool -find_dependent_phvs(PlannerInfo *root, int varno) +find_dependent_phvs(PlannerInfo *root, int varno, Relids baserels) { find_dependent_phvs_context context; @@ -4341,6 +4388,7 @@ find_dependent_phvs(PlannerInfo *root, int varno) return false; context.relids = bms_make_singleton(varno); + context.baserels = baserels; context.sublevels_up = 0; if (query_tree_walker(root->parse, find_dependent_phvs_walker, &context, 0)) @@ -4354,7 +4402,8 @@ find_dependent_phvs(PlannerInfo *root, int varno) } static bool -find_dependent_phvs_in_jointree(PlannerInfo *root, Node *node, int varno) +find_dependent_phvs_in_jointree(PlannerInfo *root, Node *node, int varno, + Relids baserels) { find_dependent_phvs_context context; Relids subrelids; @@ -4365,6 +4414,7 @@ find_dependent_phvs_in_jointree(PlannerInfo *root, Node *node, int varno) return false; context.relids = bms_make_singleton(varno); + context.baserels = baserels; context.sublevels_up = 0; /* diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 83bd5649d5c..65fc3c820bc 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -2630,6 +2630,34 @@ select * from int4_tbl t1 -> Seq Scan on tenk1 t4 (14 rows) +-- check a case where we formerly failed to detect that a PlaceHolderVar +-- containing a constant should still be nulled by an outer join above the +-- one that was removed by remove_useless_result_rtes +explain (verbose, costs off) +select * from (values (1),(2)) v(x) + left join (select q from (select 7 as q from (select where false) ss1) ss2 + left join (select 8 as z) ss3 on true) ss4 on true; + QUERY PLAN +------------------------------------ + Nested Loop Left Join + Output: "*VALUES*".column1, (7) + Join Filter: false + -> Values Scan on "*VALUES*" + Output: "*VALUES*".column1 + -> Result + Output: 7 + One-Time Filter: false +(8 rows) + +select * from (values (1),(2)) v(x) + left join (select q from (select 7 as q from (select where false) ss1) ss2 + left join (select 8 as z) ss3 on true) ss4 on true; + x | q +---+--- + 1 | + 2 | +(2 rows) + explain (costs off) select * from onek t1 left join onek t2 on t1.unique1 = t2.unique1 diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 32d4a5a677e..af12542b4ff 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -531,6 +531,18 @@ select * from int4_tbl t1 left join tenk1 t4 on s.f1 > 1) on s.f1 = t1.f1; +-- check a case where we formerly failed to detect that a PlaceHolderVar +-- containing a constant should still be nulled by an outer join above the +-- one that was removed by remove_useless_result_rtes +explain (verbose, costs off) +select * from (values (1),(2)) v(x) + left join (select q from (select 7 as q from (select where false) ss1) ss2 + left join (select 8 as z) ss3 on true) ss4 on true; + +select * from (values (1),(2)) v(x) + left join (select q from (select 7 as q from (select where false) ss1) ss2 + left join (select 8 as z) ss3 on true) ss4 on true; + explain (costs off) select * from onek t1 left join onek t2 on t1.unique1 = t2.unique1 -- 2.50.1 (Apple Git-155)