agora inbox for pgsql-bugs@postgresql.org
help / color / mirror / Atom feedFrom: Tom Lane <tgl@sss.pgh.pa.us>
To: Matheus Alcantara <matheusssilv97@gmail.com>
Cc: leis@in.tum.de, pgsql-bugs@lists.postgresql.org, "Richard Guo" <guofenglinux@gmail.com>
Subject: Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
Date: Fri, 17 Jul 2026 13:20:23 -0400
Message-ID: <3957687.1784308823@sss.pgh.pa.us> (raw)
In-Reply-To: <DK0XT8FDBCLK.A43UXISCEHW9@gmail.com>
References: <19553-4561747f93f368a7@postgresql.org>
<DK0CT1H1TW0G.3AJAOZTZJVTLL@gmail.com>
<3824828.1784244210@sss.pgh.pa.us>
<DK0XT8FDBCLK.A43UXISCEHW9@gmail.com>
"Matheus Alcantara" <matheusssilv97@gmail.com> 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
Attachments:
[text/x-diff] v3-0001-Fix-edge-case-in-remove_useless_result_rtes-with-.patch (11.6K, ../3957687.1784308823@sss.pgh.pa.us/2-v3-0001-Fix-edge-case-in-remove_useless_result_rtes-with-.patch)
download | inline diff:
From 93c43dd3fffb45e7faa77d48b6b36b1d32a531a1 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
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 <leis@in.tum.de>
Author: Matheus Alcantara <matheusssilv97@gmail.com>
Co-authored-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
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
view thread (13+ messages) latest in thread
Message-ID: <3957687.1784308823@sss.pgh.pa.us>
Permalink: ../3957687.1784308823@sss.pgh.pa.us/
Also on: postgresql.org/message-id/3957687.1784308823@sss.pgh.pa.us
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, matheusssilv97@gmail.com, guofenglinux@gmail.com
Subject: Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
In-Reply-To: <3957687.1784308823@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox