BUG #19553: Wrong results from nested LEFT JOINs over an empty subquery (regression since v16)

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

"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

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

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

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

"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

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

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

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

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

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

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