Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wkmEo-000rx3-1e for pgsql-bugs@arkaria.postgresql.org; Fri, 17 Jul 2026 17:20:38 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wkmEn-001LrY-0q for pgsql-bugs@arkaria.postgresql.org; Fri, 17 Jul 2026 17:20:37 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wkmEm-001LrP-33 for pgsql-bugs@lists.postgresql.org; Fri, 17 Jul 2026 17:20:37 +0000 Received: from sss.pgh.pa.us ([68.162.161.243]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.98.2) (envelope-from ) id 1wkmEg-00000000nf3-3c2H for pgsql-bugs@lists.postgresql.org; Fri, 17 Jul 2026 17:20:36 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.18.1/8.18.1) with ESMTP id 66HHKNex3957688; Fri, 17 Jul 2026 13:20:23 -0400 From: Tom Lane To: "Matheus Alcantara" cc: leis@in.tum.de, pgsql-bugs@lists.postgresql.org, "Richard Guo" Subject: Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16) In-reply-to: References: <19553-4561747f93f368a7@postgresql.org> <3824828.1784244210@sss.pgh.pa.us> Comments: In-reply-to "Matheus Alcantara" message dated "Fri, 17 Jul 2026 12:05:06 -0300" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <3957624.1784308777.0@sss.pgh.pa.us> Date: Fri, 17 Jul 2026 13:20:23 -0400 Message-ID: <3957687.1784308823@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <3957624.1784308777.1@sss.pgh.pa.us> "Matheus Alcantara" writes: > On Thu Jul 16, 2026 at 8:23 PM -03, Tom Lane wrote: >> So the simplest fix is probably to mask off OJ bits and consider only >> baserels when deciding if phrels equals the target. > Attached patch does this: remove_useless_result_rtes() now precomputes a > Relids of all non-join RT indexes by scanning root->parse->rtable once > up front (rather than doing a second recursive walk), and threads it > through remove_useless_results_recurse() down to both > find_dependent_phvs() and find_dependent_phvs_in_jointree(). I think that Richard's suggestion of using get_relids_in_jointree is superior: the rtable may contain RTEs that are no longer relevant, such as views that have been expanded. In principle including those in the mask wouldn't make a difference, but it seems messy. I was thinking yesterday that we could calculate the mask on the fly because by the time we are making decisions at a join node, we've already recursively visited all its children, and those should include all the baserels that are relevant. But I do agree with Richard that that "should" feels maybe a wee bit shaky, and it's not like the join tree would be so big here that we can't afford one more recursive traversal. On the third hand, I also find Richard's suggestion of relying on dropped_outer_joins to be shaky. It's far from clear to me that not-dropped outer joins couldn't also break this test. This code was designed to consider only baserels, so I think the safest route to a fix is to restore it to doing that. (Also, dropped_outer_joins seems like it suffers from the same objection that maybe it doesn't *yet* include every outer join that is relevant.) So here's a v3 that is basically Matheus' code, but with the rtable scan replaced with get_relids_in_jointree, and some other cosmetic changes. (Notably, I changed argument order to preserve our usual convention that output arguments come last.) I preferred Richard's commentary on the test case though. If no objections, I'll push and backpatch soon. Another thought for the future: at least in this example, it seems like removing the second RTE_RESULT and its parent outer join would be perfectly valid, if what we did to clean up is replace the dependent PHV(s) with null Consts. I'm not sure that such cases arise often enough to be worth the trouble (if they did we'd likely have heard about this bug long ago), but it's interesting to think about. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name*0="v3-0001-Fix-edge-case-in-remove_useless_result_rtes-with-.p"; name*1="atch"; charset="us-ascii" Content-ID: <3957624.1784308777.2@sss.pgh.pa.us> Content-Description: v3-0001-Fix-edge-case-in-remove_useless_result_rtes-with-.patch Content-Transfer-Encoding: quoted-printable =46rom 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 out= er 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/optim= izer/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_jo= ins_pass2_state *state2, static bool has_notnull_forced_var(PlannerInfo *root, List *forced_null_v= ars, reduce_outer_joins_pass1_state *right_state); static Node *remove_useless_results_recurse(PlannerInfo *root, Node *jtno= de, + 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 *newjtl= oc); -static bool find_dependent_phvs(PlannerInfo *root, int varno); +static bool find_dependent_phvs(PlannerInfo *root, int varno, Relids base= rels); 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 *fo= rced_null_vars, void remove_useless_result_rtes(PlannerInfo *root) { + Relids baserels; Relids dropped_outer_joins =3D NULL; ListCell *cell; = + /* + * We'll need the set of baserels in the jointree to perform + * find_dependent_phvs() checks. + */ + baserels =3D 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 =3D (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 jointre= e. */ 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, No= de *jtnode, = /* Recursively transform child, allowing it to push up quals ... */ child =3D 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, No= de *jtnode, */ if (list_length(f->fromlist) > 1 && (varno =3D get_result_relid(root, child)) !=3D 0 && - !find_dependent_phvs_in_jointree(root, (Node *) f, varno)) + !find_dependent_phvs_in_jointree(root, (Node *) f, varno, + baserels)) { f->fromlist =3D foreach_delete_current(f->fromlist, cell); result_relids =3D 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 =3D remove_useless_results_recurse(root, j->larg, + baserels, (j->jointype =3D=3D JOIN_INNER) ? &j->quals : (j->jointype =3D=3D JOIN_LEFT) ? parent_quals : NULL, dropped_outer_joins); j->rarg =3D remove_useless_results_recurse(root, j->rarg, + baserels, (j->jointype =3D=3D JOIN_INNER || j->jointype =3D=3D JOIN_LEFT) ? &j->quals : NULL, @@ -4103,7 +4121,8 @@ remove_useless_results_recurse(PlannerInfo *root, No= de *jtnode, * allowed to have such refs. */ if ((varno =3D get_result_relid(root, j->larg)) !=3D 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 !=3D NULL && parent_quals =3D=3D NULL) @@ -4158,7 +4177,7 @@ remove_useless_results_recurse(PlannerInfo *root, No= de *jtnode, */ if ((varno =3D get_result_relid(root, j->rarg)) !=3D 0 && (j->quals =3D=3D NULL || - !find_dependent_phvs(root, varno))) + !find_dependent_phvs(root, varno, baserels))) { remove_result_refs(root, varno, j->larg); *dropped_outer_joins =3D bms_add_member(*dropped_outer_joins, @@ -4280,9 +4299,17 @@ remove_result_refs(PlannerInfo *root, int varno, No= de *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, Nod= e *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 =3D (PlaceHolderVar *) node; = - if (phv->phlevelsup =3D=3D context->sublevels_up && - bms_equal(context->relids, phv->phrels)) - return true; + if (phv->phlevelsup =3D=3D context->sublevels_up) + { + Relids phbaserels =3D bms_intersect(phv->phrels, + context->baserels); + bool match =3D 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 =3D bms_make_singleton(varno); + context.baserels =3D baserels; context.sublevels_up =3D 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, N= ode *node, int varno) return false; = context.relids =3D bms_make_singleton(varno); + context.baserels =3D baserels; context.sublevels_up =3D 0; = /* diff --git a/src/test/regress/expected/join.out b/src/test/regress/expecte= d/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 joi= n. +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" lo= gic 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 joi= n. +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" lo= gic begin; set local from_collapse_limit to 2; -- = 2.52.0 ------- =_aaaaaaaaaa0--