agora inbox for pgsql-bugs@postgresql.org
help / color / mirror / Atom feedBUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
13+ messages / 4 participants
[nested] [flat]
* BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-16 14:12 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 13+ messages in thread
From: PG Bug reporting form @ 2026-07-16 14:12 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: leis@in.tum.de
The following bug has been logged on the website:
Bug reference: 19553
Logged by: Viktor Leis
Email address: leis@in.tum.de
PostgreSQL version: 19beta2
Operating system: Ubuntu 26.04
Description:
Hi,
The following self-contained query returns wrong results on every release
since v16:
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 | 7
2 | 7
(2 rows)
The right-hand side of the top left join is provably empty (ss1 produces no
rows, and the inner left join preserves that), so the correct result
null-extends both rows:
x | q
---+---
1 |
2 |
(2 rows)
EXPLAIN (VERBOSE) on affected versions shows that the entire RHS has been
optimized away and the constant is emitted unconditionally:
Values Scan on "*VALUES*"
Output: "*VALUES*".column1, 7
I bisected the regression to commit 3af87736bf5 ("Fix another cause of
'wrong varnullingrels' planner failures" by Tom Lane).
Claude Code Analysis:
After subquery pullup, everything is still correct. Using the RT indexes
of the example (4 = the VALUES rel, 9/10 = the RESULT rels deriving from
ss1 resp. ss3, 3/7 = the RTIs of the upper resp. inner left join), the
jointree is
VALUES(4) leftjoin[3] ( FromExpr(RESULT(9), quals=false) leftjoin[7]
RESULT(10) )
and the output column q is
PlaceHolderVar(Const 7, phrels={7,9,10}, phnullingrels={3})
Then remove_useless_results_recurse() goes wrong in three steps:
1. The mechanism added by 3af87736bf5 hoists the constant-false qual from
the single-child FromExpr (ss1's WHERE clause) through the inner join's
parent_quals pointer into the *upper* join's quals, collapsing the
FromExpr to a bare RangeTblRef of RESULT(9).
2. The inner left join (RTI 7, ON true against the one-row RESULT(10)) is
dropped; that is fine in itself. remove_result_refs() substitutes
10 -> {9} in the PHV's phrels, giving {7,9}. Crucially, the dropped
join's RTI 7 stays in phrels: cleanup of dropped-join RTIs is deferred
to a single remove_nulling_relids() pass at the end of
remove_useless_result_rtes().
3. For the upper join (RTI 3), now with quals=false over a bare RESULT(9),
removal is only legal if no PHV must be evaluated at the RESULT rel;
the guard find_dependent_phvs(root, 9) tests
bms_equal(phv->phrels, {9}). Because of the stale RTI the PHV's phrels
is {7,9}, the exact-match test misses it, and the join is dropped even
though its constant-false quals mean every LHS row must be
null-extended. remove_result_refs() then relocates the PHV to the
VALUES rel and the end-of-pass cleanup strips RTI 3 from its
phnullingrels, leaving a never-nulled Const 7.
So the guard itself is fine; it is defeated by phrels not being maintained
while the recursion is still running. Note that the end-of-pass cleanup
cannot simply be moved earlier, because remove_nulling_relids() is a
mutator that would invalidate the jointree surgery in progress.
Best regards,
Viktor Leis
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-16 22:37 Matheus Alcantara <matheusssilv97@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 13+ messages in thread
From: Matheus Alcantara @ 2026-07-16 22:37 UTC (permalink / raw)
To: leis@in.tum.de; pgsql-bugs@lists.postgresql.org
On Thu Jul 16, 2026 at 11:12 AM -03, PG Bug reporting form wrote:
> The following bug has been logged on the website:
>
> Bug reference: 19553
> Logged by: Viktor Leis
> Email address: leis@in.tum.de
> PostgreSQL version: 19beta2
> Operating system: Ubuntu 26.04
> Description:
>
> Hi,
>
> The following self-contained query returns wrong results on every release
> since v16:
>
> 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 | 7
> 2 | 7
> (2 rows)
>
> The right-hand side of the top left join is provably empty (ss1 produces no
> rows, and the inner left join preserves that), so the correct result
> null-extends both rows:
>
> x | q
> ---+---
> 1 |
> 2 |
> (2 rows)
>
> EXPLAIN (VERBOSE) on affected versions shows that the entire RHS has been
> optimized away and the constant is emitted unconditionally:
>
> Values Scan on "*VALUES*"
> Output: "*VALUES*".column1, 7
>
> I bisected the regression to commit 3af87736bf5 ("Fix another cause of
> 'wrong varnullingrels' planner failures" by Tom Lane).
>
Hi, thank you for the report and for the script reproducer.
According to my findings, the issue seems to be in
remove_useless_results_recurse(), specifically in find_dependent_phvs().
When a LEFT JOIN's RHS reduces to an RTE_RESULT, that function is used
to check whether any PlaceHolderVar still depends on the RTE_RESULT
being considered for removal; if not, the join is discarded along with
the RTE_RESULT. The check was testing whether a PHV's phrels was exactly
equal to {varno}, but that's too narrow. In your example, the constant
"7" gets wrapped in a PlaceHolderVar whose phrels ends up as the union
of both RTE_RESULTs in the inner commuted left join (the ss1 "WHERE
false" result and the ss3 "SELECT 8" result), because the pulled-up
subquery's output needs to be nullable by the outer join. Since phrels
was {ss1, ss3} rather than exactly {ss1}, find_dependent_phvs() wrongly
concluded the outer join had no remaining dependents and dropped it,
taking the join's null-extension semantics with it. That's why the
constant got emitted unconditionally instead of being nulled out for the
"no match" case.
Attached patch seems to fix find_dependent_phvs() to test membership
rather than exact equality, since that's what actually indicates the
PHV's value depends on that relation. find_dependent_phvs_in_jointree(),
used for a different purpose so keeps the exact-match behavior it had,
as that's it seems to still correct for its use case. I added a field to
the shared context struct to keep both behaviors sharing one walker.
Verified against your original repro and a version using tables instead
of VALUES/constants (make it more easier for me to debug):
create table t (id int, name text);
insert into t values (1, 'Alice'), (2, 'Bob');
select c.id, c.name, promo.discount
from t c
left join (
select discount
from (select 0.20 as discount from (select where false) no_matching_promo) rates
left join (select true as eligible) elig on true
) promo on true;
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
From 6672f3c59c17715cba4500b19bf6cb7b64b19b71 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <mths.dev@pm.me>
Date: Thu, 16 Jul 2026 19:05:55 -0300
Subject: [PATCH] Fix wrong results from remove_useless_result_rtes with nested
PHVs.
find_dependent_phvs() was checking whether any PlaceHolderVar's phrels
was exactly equal to the RTE_RESULT relid being considered for removal.
But a PHV can legitimately have additional relids in phrels beyond the
one being removed, when the PHV's value is computed jointly with a
sibling RTE_RESULT that gets folded away first, as happens when two
commutable left joins are nested and the lower one's RHS reduces to a
constant. In such cases the PHV still depends on the outer RTE_RESULT
being removed, but the exact-match test missed it, causing
remove_useless_results_recurse to discard the enclosing left join along
with the RTE_RESULT. That silently dropped the outer join's
null-extension semantics, so a PHV-wrapped constant that should have
been nulled out for unmatched rows was instead emitted unconditionally
for every row.
Fix find_dependent_phvs to test whether the given relid is a member of
phrels rather than requiring an exact match, since membership is what
actually indicates a dependency. find_dependent_phvs_in_jointree, which
is used for a different purpose (deciding whether a FromExpr or inner
join can be elided) and has different correctness requirements, keeps
its original exact-match behavior. The two now share the walker via an
added "exact" flag on the context struct.
Author: Matheus Alcantara <mths.dev@pm.me>
Reported-by: leis@in.tum.de <leis@in.tum.de>
Discussion: https://www.postgresql.org/message-id/19553-4561747f93f368a7@postgresql.org
---
src/backend/optimizer/prep/prepjointree.c | 23 ++++++++++++++-----
src/test/regress/expected/join.out | 28 +++++++++++++++++++++++
src/test/regress/sql/join.sql | 12 ++++++++++
3 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index ca5ca8bfe22..188eccea4d5 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -4280,20 +4280,27 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc)
/*
- * find_dependent_phvs - are there any PlaceHolderVars whose relids are
- * exactly the given varno?
+ * find_dependent_phvs - are there any PlaceHolderVars that depend on the
+ * given varno?
+ *
+ * "Depend on" means the varno is a member of the PHV's phrels, i.e. the PHV's
+ * value is (partly) computed at that relation. We must consider such PHVs and
+ * not only those whose phrels are exactly {varno}, because a left join whose
+ * RHS is that relation nulls the whole PHV; dropping the join would silently
+ * un-null it. See the JOIN_LEFT case in remove_useless_results_recurse.
*
* 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
- * to look not only at the join tree nodes themselves but at the
- * referenced RTEs. For that, use find_dependent_phvs_in_jointree.
+ * a subtree of the join tree contains PHVs whose phrels are exactly {varno};
+ * but for that, we have to look not only at the join tree nodes themselves but
+ * at the referenced RTEs. For that, use find_dependent_phvs_in_jointree.
*/
typedef struct
{
Relids relids;
int sublevels_up;
+ bool exact; /* match phrels exactly, else by membership */
} find_dependent_phvs_context;
static bool
@@ -4307,7 +4314,9 @@ find_dependent_phvs_walker(Node *node,
PlaceHolderVar *phv = (PlaceHolderVar *) node;
if (phv->phlevelsup == context->sublevels_up &&
- bms_equal(context->relids, phv->phrels))
+ (context->exact ?
+ bms_equal(context->relids, phv->phrels) :
+ bms_is_subset(context->relids, phv->phrels)))
return true;
/* fall through to examine children */
}
@@ -4342,6 +4351,7 @@ find_dependent_phvs(PlannerInfo *root, int varno)
context.relids = bms_make_singleton(varno);
context.sublevels_up = 0;
+ context.exact = false;
if (query_tree_walker(root->parse, find_dependent_phvs_walker, &context, 0))
return true;
@@ -4366,6 +4376,7 @@ find_dependent_phvs_in_jointree(PlannerInfo *root, Node *node, int varno)
context.relids = bms_make_singleton(varno);
context.sublevels_up = 0;
+ context.exact = true;
/*
* See if the jointree fragment itself contains references (in join quals)
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)
Attachments:
[text/plain] 0001-Fix-wrong-results-from-remove_useless_result_rtes-wi.patch (6.8K, ../../DK0CT1H1TW0G.3AJAOZTZJVTLL@gmail.com/2-0001-Fix-wrong-results-from-remove_useless_result_rtes-wi.patch)
download | inline diff:
From 6672f3c59c17715cba4500b19bf6cb7b64b19b71 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <mths.dev@pm.me>
Date: Thu, 16 Jul 2026 19:05:55 -0300
Subject: [PATCH] Fix wrong results from remove_useless_result_rtes with nested
PHVs.
find_dependent_phvs() was checking whether any PlaceHolderVar's phrels
was exactly equal to the RTE_RESULT relid being considered for removal.
But a PHV can legitimately have additional relids in phrels beyond the
one being removed, when the PHV's value is computed jointly with a
sibling RTE_RESULT that gets folded away first, as happens when two
commutable left joins are nested and the lower one's RHS reduces to a
constant. In such cases the PHV still depends on the outer RTE_RESULT
being removed, but the exact-match test missed it, causing
remove_useless_results_recurse to discard the enclosing left join along
with the RTE_RESULT. That silently dropped the outer join's
null-extension semantics, so a PHV-wrapped constant that should have
been nulled out for unmatched rows was instead emitted unconditionally
for every row.
Fix find_dependent_phvs to test whether the given relid is a member of
phrels rather than requiring an exact match, since membership is what
actually indicates a dependency. find_dependent_phvs_in_jointree, which
is used for a different purpose (deciding whether a FromExpr or inner
join can be elided) and has different correctness requirements, keeps
its original exact-match behavior. The two now share the walker via an
added "exact" flag on the context struct.
Author: Matheus Alcantara <mths.dev@pm.me>
Reported-by: leis@in.tum.de <leis@in.tum.de>
Discussion: https://www.postgresql.org/message-id/19553-4561747f93f368a7@postgresql.org
---
src/backend/optimizer/prep/prepjointree.c | 23 ++++++++++++++-----
src/test/regress/expected/join.out | 28 +++++++++++++++++++++++
src/test/regress/sql/join.sql | 12 ++++++++++
3 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index ca5ca8bfe22..188eccea4d5 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -4280,20 +4280,27 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc)
/*
- * find_dependent_phvs - are there any PlaceHolderVars whose relids are
- * exactly the given varno?
+ * find_dependent_phvs - are there any PlaceHolderVars that depend on the
+ * given varno?
+ *
+ * "Depend on" means the varno is a member of the PHV's phrels, i.e. the PHV's
+ * value is (partly) computed at that relation. We must consider such PHVs and
+ * not only those whose phrels are exactly {varno}, because a left join whose
+ * RHS is that relation nulls the whole PHV; dropping the join would silently
+ * un-null it. See the JOIN_LEFT case in remove_useless_results_recurse.
*
* 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
- * to look not only at the join tree nodes themselves but at the
- * referenced RTEs. For that, use find_dependent_phvs_in_jointree.
+ * a subtree of the join tree contains PHVs whose phrels are exactly {varno};
+ * but for that, we have to look not only at the join tree nodes themselves but
+ * at the referenced RTEs. For that, use find_dependent_phvs_in_jointree.
*/
typedef struct
{
Relids relids;
int sublevels_up;
+ bool exact; /* match phrels exactly, else by membership */
} find_dependent_phvs_context;
static bool
@@ -4307,7 +4314,9 @@ find_dependent_phvs_walker(Node *node,
PlaceHolderVar *phv = (PlaceHolderVar *) node;
if (phv->phlevelsup == context->sublevels_up &&
- bms_equal(context->relids, phv->phrels))
+ (context->exact ?
+ bms_equal(context->relids, phv->phrels) :
+ bms_is_subset(context->relids, phv->phrels)))
return true;
/* fall through to examine children */
}
@@ -4342,6 +4351,7 @@ find_dependent_phvs(PlannerInfo *root, int varno)
context.relids = bms_make_singleton(varno);
context.sublevels_up = 0;
+ context.exact = false;
if (query_tree_walker(root->parse, find_dependent_phvs_walker, &context, 0))
return true;
@@ -4366,6 +4376,7 @@ find_dependent_phvs_in_jointree(PlannerInfo *root, Node *node, int varno)
context.relids = bms_make_singleton(varno);
context.sublevels_up = 0;
+ context.exact = true;
/*
* See if the jointree fragment itself contains references (in join quals)
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)
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-16 23:23 Tom Lane <tgl@sss.pgh.pa.us>
parent: Matheus Alcantara <matheusssilv97@gmail.com>
0 siblings, 3 replies; 13+ messages in thread
From: Tom Lane @ 2026-07-16 23:23 UTC (permalink / raw)
To: Matheus Alcantara <matheusssilv97@gmail.com>; +Cc: leis@in.tum.de; pgsql-bugs@lists.postgresql.org
"Matheus Alcantara" <matheusssilv97@gmail.com> writes:
> On Thu Jul 16, 2026 at 11:12 AM -03, PG Bug reporting form wrote:
>> The following self-contained query returns wrong results on every release
>> since v16:
>>
>> 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;
> According to my findings, the issue seems to be in
> remove_useless_results_recurse(), specifically in find_dependent_phvs().
Yeah, I had just come to the same conclusion: we are deciding that the
PHV is not dependent on the RTE_RESULT we're considering removing, but
it really is.
> Attached patch seems to fix find_dependent_phvs() to test membership
> rather than exact equality, since that's what actually indicates the
> PHV's value depends on that relation.
I had thought of that too, but I think it is wrong and will result in
not pursuing optimizations that are valid. I instrumented the code
like this:
*** 4309,4314 ****
--- 4309,4319 ----
if (phv->phlevelsup == context->sublevels_up &&
bms_equal(context->relids, phv->phrels))
return true;
+ if (phv->phlevelsup == context->sublevels_up &&
+ bms_is_subset(context->relids, phv->phrels))
+ elog(WARNING, "dubious case detected: looking for %s, PHV has %s",
+ bmsToString(context->relids),
+ bmsToString(phv->phrels));
/* fall through to examine children */
}
and observed that this warning fires in several join.sql cases that
are not giving wrong answers. (Unfortunately, those tests only check
the query results not the plan, so they'd not show any change in
behavior from your patch.)
What I see here is that the PHV in question initially has
:phrels (b 7 9 10) -- lower OJ, RESULT, RESULT
:phnullingrels (b 3) -- upper OJ
and we decide that RTE 10 can be removed, leaving
:phrels (b 7 9) -- lower OJ, RESULT
:phnullingrels (b 3) -- upper OJ
That's fine, but when we come to consider RTE 9, we decide it can be
removed, which is wrong. I think the core of the problem here is that
this code was written back when phrels contained only baserels, and
now that it also contains OJ rels, we're mistakenly concluding that
the presence of those bits indicates there's another place to evaluate
the PHV.
So the simplest fix is probably to mask off OJ bits and consider only
baserels when deciding if phrels equals the target. Unfortunately,
this happens long before we compute root->all_baserels or anything
like that, so remove_useless_result_rtes is on its own to figure out
which those are. I think we can extend it to build a bitmapset of
relevant baserel RT indexes while it is scanning the tree (so that we
don't need an additional recursive scan just to get that). But I've
not tried to write any code yet; do you feel like attacking that?
find_dependent_phvs_in_jointree most likely needs the same fix.
I don't believe your conclusion that it should act differently.
BTW, I think "git bisect"'s finding that the bug started with
commit 3af87736b is mostly accidental. That commit removed a
different limitation preventing the intermediate FromExpr from
getting flattened, allowing the problem to be reached.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-16 23:47 Matheus Alcantara <matheusssilv97@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 0 replies; 13+ messages in thread
From: Matheus Alcantara @ 2026-07-16 23:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: leis@in.tum.de; pgsql-bugs@lists.postgresql.org
On Thu Jul 16, 2026 at 8:23 PM -03, Tom Lane wrote:
>
> [ ... ]
>
> But I've not tried to write any code yet; do you feel like attacking
> that?
>
Sure, I'll try to write and I'll share soon.
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-17 08:55 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 0 replies; 13+ messages in thread
From: Richard Guo @ 2026-07-17 08:55 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Matheus Alcantara <matheusssilv97@gmail.com>; leis@in.tum.de; pgsql-bugs@lists.postgresql.org
On Fri, Jul 17, 2026 at 8:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So the simplest fix is probably to mask off OJ bits and consider only
> baserels when deciding if phrels equals the target. Unfortunately,
> this happens long before we compute root->all_baserels or anything
> like that, so remove_useless_result_rtes is on its own to figure out
> which those are. I think we can extend it to build a bitmapset of
> relevant baserel RT indexes while it is scanning the tree (so that we
> don't need an additional recursive scan just to get that).
It seems to me that with this approach the baserel RT indexes may not
be complete when a given PHV is examined, and I'm worried that that
could cause us to lose some optimizations. Maybe we can just call
get_relids_in_jointree((Node *) root->parse->jointree, false, false)
to get all baserel relids up front, though that costs an extra
recursive scan of the parsetree.
While looking into this, I was a little surprised that when we come to
consider RTE 9, phrels still contains ojrelid 7. I'd have expected
that to go away when we remove RTE 10, since the lower OJ is dropped
at the same time. The comment explains why it doesn't:
* ... We
* don't do this during the main recursion, for simplicity and because we
* can handle all such joins in a single pass over the parse tree.
So I'm thinking that we can subtract dropped_outer_joins from phrels
before comparing. The outer-join relids that show up here belong to
joins we have already decided to drop; their relids are stale only
because phrels does not get fixed up until the end of the pass. And a
surviving outer join in phrels always brings other baserels along with
it, so it cannot cause a false match. Also, the "dropped_outer_joins"
is already built up in remove_useless_results_recurse, so it is right
at hand.
I tried this idea and ended up with the attached.
- Richard
Attachments:
[application/octet-stream] v2-0001-Fix-RTE_RESULT-removal-to-disregard-stale-outer-j.patch (8.0K, ../../CAMbWs4-+rfUg5j6G_GwLEj9HbV4deAo+9Ta9Ok3-6GZZXRddYg@mail.gmail.com/2-v2-0001-Fix-RTE_RESULT-removal-to-disregard-stale-outer-j.patch)
download | inline diff:
From 2aa51786a037bc4e34041aabe478eb794815ce4b Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Fri, 17 Jul 2026 17:04:31 +0900
Subject: [PATCH v2] Fix RTE_RESULT removal to disregard stale outer-join
relids in phrels
---
src/backend/optimizer/prep/prepjointree.c | 46 +++++++++++++++++------
src/test/regress/expected/join.out | 27 +++++++++++++
src/test/regress/sql/join.sql | 10 +++++
3 files changed, 72 insertions(+), 11 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index ca5ca8bfe22..5d9c29f8d85 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -170,9 +170,11 @@ static Node *remove_useless_results_recurse(PlannerInfo *root, Node *jtnode,
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 dropped_outer_joins);
static bool find_dependent_phvs_in_jointree(PlannerInfo *root,
- Node *node, int varno);
+ Node *node, int varno,
+ Relids dropped_outer_joins);
static void substitute_phv_relids(Node *node,
int varno, Relids subrelids);
static void fix_append_rel_relids(PlannerInfo *root, int varno,
@@ -3948,7 +3950,10 @@ remove_useless_result_rtes(PlannerInfo *root)
*
* This recursively processes the jointree and returns a modified jointree.
* In addition, the RT indexes of any removed outer-join nodes are added to
- * *dropped_outer_joins.
+ * *dropped_outer_joins. Besides being fixed up at the end of the pass,
+ * that set is consulted while we recurse: it lets us mask stale outer-join
+ * relids out of a PlaceHolderVar's phrels when checking whether a PHV still
+ * needs the RTE_RESULT we're considering removing.
*
* jtnode is the current jointree node. If it could be valid to merge
* its quals into those of the parent node, parent_quals should point to
@@ -4001,7 +4006,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,
+ *dropped_outer_joins))
{
f->fromlist = foreach_delete_current(f->fromlist, cell);
result_relids = bms_add_member(result_relids, varno);
@@ -4103,7 +4109,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,
+ *dropped_outer_joins))
{
remove_result_refs(root, varno, j->rarg);
if (j->quals != NULL && parent_quals == NULL)
@@ -4158,7 +4165,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, *dropped_outer_joins)))
{
remove_result_refs(root, varno, j->larg);
*dropped_outer_joins = bms_add_member(*dropped_outer_joins,
@@ -4288,11 +4295,16 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc)
* a subtree of the join tree contains such PHVs; but for that, we have
* to look not only at the join tree nodes themselves but at the
* referenced RTEs. For that, use find_dependent_phvs_in_jointree.
+ *
+ * A PHV's phrels can also contain outer-join relids. We ignore any that
+ * refer to an already-dropped outer join (passed in dropped_outer_joins),
+ * since those are stale until phrels is fixed up at the end of the pass.
*/
typedef struct
{
Relids relids;
+ Relids dropped_outer_joins;
int sublevels_up;
} find_dependent_phvs_context;
@@ -4306,9 +4318,18 @@ 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 phrels = phv->phrels;
+
+ phrels = bms_difference(phrels, context->dropped_outer_joins);
+ if (bms_equal(context->relids, phrels))
+ {
+ bms_free(phrels);
+ return true;
+ }
+ bms_free(phrels);
+ }
/* fall through to examine children */
}
if (IsA(node, Query))
@@ -4332,7 +4353,7 @@ find_dependent_phvs_walker(Node *node,
}
static bool
-find_dependent_phvs(PlannerInfo *root, int varno)
+find_dependent_phvs(PlannerInfo *root, int varno, Relids dropped_outer_joins)
{
find_dependent_phvs_context context;
@@ -4341,6 +4362,7 @@ find_dependent_phvs(PlannerInfo *root, int varno)
return false;
context.relids = bms_make_singleton(varno);
+ context.dropped_outer_joins = dropped_outer_joins;
context.sublevels_up = 0;
if (query_tree_walker(root->parse, find_dependent_phvs_walker, &context, 0))
@@ -4354,7 +4376,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 dropped_outer_joins)
{
find_dependent_phvs_context context;
Relids subrelids;
@@ -4365,6 +4388,7 @@ find_dependent_phvs_in_jointree(PlannerInfo *root, Node *node, int varno)
return false;
context.relids = bms_make_singleton(varno);
+ context.dropped_outer_joins = dropped_outer_joins;
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.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-17 15:05 Matheus Alcantara <matheusssilv97@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 13+ messages in thread
From: Matheus Alcantara @ 2026-07-17 15:05 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: leis@in.tum.de; pgsql-bugs@lists.postgresql.org; Richard Guo <guofenglinux@gmail.com>
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. Unfortunately,
> this happens long before we compute root->all_baserels or anything
> like that, so remove_useless_result_rtes is on its own to figure out
> which those are. I think we can extend it to build a bitmapset of
> relevant baserel RT indexes while it is scanning the tree (so that we
> don't need an additional recursive scan just to get that). But I've
> not tried to write any code yet; do you feel like attacking that?
>
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(). Both now
intersect a candidate PHV's phrels with that baserels set before
comparing to the target relid, instead of comparing phrels as-is.
I checked that a plain flat scan of the rtable is sufficient here rather
than needing to accumulate the set incrementally during the jointree
recursion. Every PHV this code ever matches has phlevelsup ==
sublevels_up, which by construction means its phrels are relids in the
outermost query's rangetable, not some inner subquery's. So a single
baserels set computed once from root->parse->rtable is valid at every
recursion depth, and we don't need to recompute or thread a different
set per query level.
Note: Richard have also shared a fix for this bug on this thread, it's
also seems to fix the issue and IIUC it fixes without the extra loop to
collect the baserels. I decided to continue with my attempt following
your suggestion so we can discuss both approaches (and also because I'm
still not super familiar with this part of the code and I wanted to
learn more about it).
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
From 79df439cd98f2e408ff54e760b3d1cdc68e4c8c8 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <mths.dev@pm.me>
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 <mths.dev@pm.me>
Reported-by: leis@in.tum.de <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)
Attachments:
[text/plain] v2-0001-Fix-wrong-results-from-remove_useless_result_rtes.patch (12.9K, ../../DK0XT8FDBCLK.A43UXISCEHW9@gmail.com/2-v2-0001-Fix-wrong-results-from-remove_useless_result_rtes.patch)
download | inline diff:
From 79df439cd98f2e408ff54e760b3d1cdc68e4c8c8 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <mths.dev@pm.me>
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 <mths.dev@pm.me>
Reported-by: leis@in.tum.de <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)
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-17 17:20 Tom Lane <tgl@sss.pgh.pa.us>
parent: Matheus Alcantara <matheusssilv97@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2026-07-17 17:20 UTC (permalink / raw)
To: Matheus Alcantara <matheusssilv97@gmail.com>; +Cc: leis@in.tum.de, pgsql-bugs@lists.postgresql.org, "Richard Guo" <guofenglinux@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
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-18 01:40 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 13+ messages in thread
From: Richard Guo @ 2026-07-18 01:40 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Matheus Alcantara <matheusssilv97@gmail.com>; leis@in.tum.de; pgsql-bugs@lists.postgresql.org
On Sat, Jul 18, 2026 at 2:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> If no objections, I'll push and backpatch soon.
v3 LGTM.
- Richard
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-18 18:11 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 2 replies; 13+ messages in thread
From: Tom Lane @ 2026-07-18 18:11 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Matheus Alcantara <matheusssilv97@gmail.com>; leis@in.tum.de; pgsql-bugs@lists.postgresql.org
Richard Guo <guofenglinux@gmail.com> writes:
> On Sat, Jul 18, 2026 at 2:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If no objections, I'll push and backpatch soon.
> v3 LGTM.
Pushed.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-18 18:33 Matheus Alcantara <matheusssilv97@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 13+ messages in thread
From: Matheus Alcantara @ 2026-07-18 18:33 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Richard Guo <guofenglinux@gmail.com>; +Cc: leis@in.tum.de; pgsql-bugs@lists.postgresql.org
On 18/07/26 15:11, Tom Lane wrote:
> Richard Guo <guofenglinux@gmail.com> writes:
>> On Sat, Jul 18, 2026 at 2:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> If no objections, I'll push and backpatch soon.
>
>> v3 LGTM.
>
> Pushed.
>
Thank you!
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-20 02:33 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 13+ messages in thread
From: Richard Guo @ 2026-07-20 02:33 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Matheus Alcantara <matheusssilv97@gmail.com>; leis@in.tum.de; pgsql-bugs@lists.postgresql.org
On Sun, Jul 19, 2026 at 3:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pushed.
It occurred to me that we can skip the new get_relids_in_jointree()
scan altogether when there are no PHVs anywhere in the query
(root->glob->lastPHId==0), since then the find_dependent_phvs() checks
are no-ops anyway. This is also consistent with how we check
root->glob->lastPHId in find_dependent_phvs() and
find_dependent_phvs_in_jointree(). Attached is a trivial patch doing
that.
- Richard
Attachments:
[application/octet-stream] v1-0001-Skip-unnecessary-get_relids_in_jointree-when-ther.patch (2.9K, ../../CAMbWs49H275KzgZr3Cd1Hy+6Lmwp35bZ+5PrVc62k3HDLj6hNQ@mail.gmail.com/2-v1-0001-Skip-unnecessary-get_relids_in_jointree-when-ther.patch)
download | inline diff:
From 2a2134c07ebb3e53108f695570f2cb3df59f799a Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 20 Jul 2026 11:11:11 +0900
Subject: [PATCH v1] Skip unnecessary get_relids_in_jointree() when there are
no PHVs
Commit 1df9e8d96 made remove_useless_result_rtes() compute the set of
baserels in the jointree, to pass down to the find_dependent_phvs()
checks. But those checks are no-ops when the query contains no PHVs,
since find_dependent_phvs() and find_dependent_phvs_in_jointree() both
return early in that case. So we can avoid the
get_relids_in_jointree() scan altogether when root->glob->lastPHId is
zero, leaving baserels as NULL.
---
src/backend/optimizer/prep/prepjointree.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 1ea72af7c73..53432c2f648 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -3888,16 +3888,18 @@ has_notnull_forced_var(PlannerInfo *root, List *forced_null_vars,
void
remove_useless_result_rtes(PlannerInfo *root)
{
- Relids baserels;
+ Relids baserels = NULL;
Relids dropped_outer_joins = NULL;
ListCell *cell;
/*
* We'll need the set of baserels in the jointree to perform
- * find_dependent_phvs() checks.
+ * find_dependent_phvs() checks. But if there are no PHVs anywhere in the
+ * query, those checks are no-ops, so we can skip the work.
*/
- baserels = get_relids_in_jointree((Node *) root->parse->jointree,
- false, false);
+ if (root->glob->lastPHId != 0)
+ 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));
@@ -4308,7 +4310,9 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc)
* 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.
+ * to evaluate a PHV, and we mustn't let that go to empty. (The caller is
+ * allowed to pass baserels as NULL if the query contains no PHVs at all,
+ * since then there is no work to do anyway.)
*
* 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
@@ -4320,7 +4324,7 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc)
typedef struct
{
Relids relids; /* target relid, represented as a relid set */
- Relids baserels; /* set of base (non-OJ) RT indexes in query */
+ Relids baserels; /* base RT indexes in query, NULL if no PHVs */
int sublevels_up; /* current nesting level */
} find_dependent_phvs_context;
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-20 02:35 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2026-07-20 02:35 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Matheus Alcantara <matheusssilv97@gmail.com>; leis@in.tum.de; pgsql-bugs@lists.postgresql.org
Richard Guo <guofenglinux@gmail.com> writes:
> It occurred to me that we can skip the new get_relids_in_jointree()
> scan altogether when there are no PHVs anywhere in the query
> (root->glob->lastPHId==0), since then the find_dependent_phvs() checks
> are no-ops anyway. This is also consistent with how we check
> root->glob->lastPHId in find_dependent_phvs() and
> find_dependent_phvs_in_jointree(). Attached is a trivial patch doing
> that.
WFM.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)
@ 2026-07-20 03:23 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 13+ messages in thread
From: Richard Guo @ 2026-07-20 03:23 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Matheus Alcantara <matheusssilv97@gmail.com>; leis@in.tum.de; pgsql-bugs@lists.postgresql.org
On Mon, Jul 20, 2026 at 11:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Richard Guo <guofenglinux@gmail.com> writes:
> > It occurred to me that we can skip the new get_relids_in_jointree()
> > scan altogether when there are no PHVs anywhere in the query
> > (root->glob->lastPHId==0), since then the find_dependent_phvs() checks
> > are no-ops anyway. This is also consistent with how we check
> > root->glob->lastPHId in find_dependent_phvs() and
> > find_dependent_phvs_in_jointree(). Attached is a trivial patch doing
> > that.
> WFM.
Thanks! Pushed.
- Richard
^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2026-07-20 03:23 UTC | newest]
Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-07-16 14:12 BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16) PG Bug reporting form <noreply@postgresql.org>
2026-07-16 22:37 ` Matheus Alcantara <matheusssilv97@gmail.com>
2026-07-16 23:23 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-07-16 23:47 ` Matheus Alcantara <matheusssilv97@gmail.com>
2026-07-17 08:55 ` Richard Guo <guofenglinux@gmail.com>
2026-07-17 15:05 ` Matheus Alcantara <matheusssilv97@gmail.com>
2026-07-17 17:20 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-07-18 01:40 ` Richard Guo <guofenglinux@gmail.com>
2026-07-18 18:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-07-18 18:33 ` Matheus Alcantara <matheusssilv97@gmail.com>
2026-07-20 02:33 ` Richard Guo <guofenglinux@gmail.com>
2026-07-20 02:35 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-07-20 03:23 ` Richard Guo <guofenglinux@gmail.com>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox