From 93c43dd3fffb45e7faa77d48b6b36b1d32a531a1 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 17 Jul 2026 12:46:48 -0400 Subject: [PATCH v3] Fix edge case in remove_useless_result_rtes() with outer joins. find_dependent_phvs() and find_dependent_phvs_in_jointree() decide whether a PlaceHolderVar depends on the RTE_RESULT rel we're considering removing by comparing the PHV's phrels to a singleton set containing that rel's RT index, reasoning that if phrels contains any other relid bits then those define an appropriate place where we can evaluate the PHV. But since this code was originally written, we've redefined phrels to include outer-join relids, and that breaks this logic, potentially allowing us to remove an RTE_RESULT that leaves no valid place to evaluate the PHV. In the known test case for this bug, the "extra" OJ relid is one that we've actually decided to remove but haven't yet cleaned out of the query's PHVs. It's not entirely clear though that that would always be the case. Let's restore this code to the way it was designed to work, by considering only base relids within the PHV's phrels. Bug: #19553 Reported-by: Viktor Leis Author: Matheus Alcantara Co-authored-by: Richard Guo Reviewed-by: Tom Lane Discussion: https://postgr.es/m/19553-4561747f93f368a7@postgresql.org Backpatch-through: 16 --- src/backend/optimizer/prep/prepjointree.c | 64 ++++++++++++++++++----- src/test/regress/expected/join.out | 27 ++++++++++ src/test/regress/sql/join.sql | 10 ++++ 3 files changed, 88 insertions(+), 13 deletions(-) diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index ca5ca8bfe22..1ea72af7c73 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -166,13 +166,15 @@ static void report_reduced_full_join(reduce_outer_joins_pass2_state *state2, 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, + Relids baserels, Node **parent_quals, Relids *dropped_outer_joins); 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, @@ -3886,15 +3888,24 @@ has_notnull_forced_var(PlannerInfo *root, List *forced_null_vars, void remove_useless_result_rtes(PlannerInfo *root) { + Relids baserels; Relids dropped_outer_joins = NULL; ListCell *cell; + /* + * We'll need the set of baserels in the jointree to perform + * find_dependent_phvs() checks. + */ + baserels = get_relids_in_jointree((Node *) root->parse->jointree, + false, false); + /* Top level of jointree must always be a FromExpr */ Assert(IsA(root->parse->jointree, FromExpr)); /* Recurse ... */ root->parse->jointree = (FromExpr *) remove_useless_results_recurse(root, (Node *) root->parse->jointree, + baserels, NULL, &dropped_outer_joins); /* We should still have a FromExpr */ @@ -3955,9 +3966,12 @@ 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 jointree. */ static Node * remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, + Relids baserels, Node **parent_quals, Relids *dropped_outer_joins) { @@ -3988,6 +4002,7 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, /* Recursively transform child, allowing it to push up quals ... */ child = remove_useless_results_recurse(root, child, + baserels, &f->quals, dropped_outer_joins); /* ... and stick it back into the tree */ @@ -4001,7 +4016,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); @@ -4070,12 +4086,14 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, * quals up, or at least there's no particular reason to. */ j->larg = remove_useless_results_recurse(root, j->larg, + baserels, (j->jointype == JOIN_INNER) ? &j->quals : (j->jointype == JOIN_LEFT) ? parent_quals : NULL, dropped_outer_joins); j->rarg = remove_useless_results_recurse(root, j->rarg, + baserels, (j->jointype == JOIN_INNER || j->jointype == JOIN_LEFT) ? &j->quals : NULL, @@ -4103,7 +4121,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 +4177,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, @@ -4280,9 +4299,17 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc) /* - * find_dependent_phvs - are there any PlaceHolderVars whose relids are + * find_dependent_phvs - are there any PlaceHolderVars whose base relids are * exactly the given varno? * + * We ignore outer-join relids present in a PHV's phrels, by intersecting + * with the caller-supplied "baserels" set. This is necessary in part + * because some of the OJ relids may be stale, that is we may have + * already decided to remove those joins in remove_useless_result_rtes + * and not yet have cleaned their relid bits out of upper PHVs. + * But in general, it's the set of baserels that identify possible places + * to evaluate a PHV, and we mustn't let that go to empty. + * * 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 @@ -4292,8 +4319,9 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc) typedef struct { - Relids relids; - int sublevels_up; + Relids relids; /* target relid, represented as a relid set */ + Relids baserels; /* set of base (non-OJ) RT indexes in query */ + int sublevels_up; /* current nesting level */ } find_dependent_phvs_context; static bool @@ -4306,9 +4334,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 +4367,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 +4376,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 +4390,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 +4402,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..19e2cca548b 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -4228,6 +4228,33 @@ select * from 1 | 2 | 2 (1 row) +-- Also, we mustn't remove an RTE_RESULT that is the only baserel where a PHV +-- can be evaluated, even when the PHV's phrels also mention an outer join. +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) + -- This example demonstrates the folly of our old "have_dangerous_phv" logic begin; set local from_collapse_limit to 2; diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 32d4a5a677e..85aed7bf704 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -1422,6 +1422,16 @@ select * from (select 1 as x) ss1 left join (select 2 as y) ss2 on (true), lateral (select ss2.y as z limit 1) ss3; +-- Also, we mustn't remove an RTE_RESULT that is the only baserel where a PHV +-- can be evaluated, even when the PHV's phrels also mention an outer join. +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; + -- This example demonstrates the folly of our old "have_dangerous_phv" logic begin; set local from_collapse_limit to 2; -- 2.52.0