remove_useless_joins vs. bug #19560

Tom Lane <tgl@sss.pgh.pa.us> 于2026年7月21日周二 02:52写道：

Anyway, what to do? I imagine we could write some code in
analyzejoins.c to consider whether ECs give rise to base restriction
clauses that they didn't before. But that seems ugly and fragile.
More generally, there is nothing that is not ugly and fragile about
analyzejoins.c's relation removal logic, and the recent addition of
self-join elimination made that situation even worse. I think we
have a permanent maintenance gotcha there.

Yes, recently, there have been a few bug fixes on HEAD related to
outer-join removal.
And there is still a bug that is not finished for self-join
elimination, for example: [1]/messages/by-id/CAHewXN=7kDJjUcgEm+6qhaKOXuqzvhRqAAKdafNCRgn0yH7BGg@mail.gmail.com
In the future, more kinds of join removal will be added. We should
think about maintenance now.

I have a modest proposal to make instead: let's nuke all that logic
from orbit. Revise analyzejoins.c so that it does join removals
working strictly on the jointree representation, which is simpler
and far more stable than any of the planner's derived data. Then,
if we successfully removed any joins, throw away all the derived
data and loop back around within query_planner() to redo
deconstruct_jointree and all the rest of it.

Current join removals based on the planner's derived data are not complete.
What I mean by complete is: for example, foo left join (bar t1 inner
join bar t2)
If "bar t1 inner join bar t2", this inner join can be removed according to SJE.
But we have no chance to perform left-join removals. We first do
left-join removals in current logic,
But the right side is not a single rel, so remove_useless_joins()
returns directly.

Can working strictly on the jointree representation handle the above case?

I'm not sure offhand what the implications would be for planning
speed. It's possible that this'd actually be faster, considering
what a mess the relation-removal logic is. But in any case, we
have a bug-prone maintenance nightmare there, and it will get
worse not better as people add more stuff to the planner. I think
the current approach is unsustainable.

Agree
In the future, inner join removal may be added, and
remove_rel_from_query() will consider 3 types of join removals at that
time.
I don't think it is easy to maintain.

[1]: /messages/by-id/CAHewXN=7kDJjUcgEm+6qhaKOXuqzvhRqAAKdafNCRgn0yH7BGg@mail.gmail.com

--
Thanks,
Tender Wang

On Tue, Jul 21, 2026 at 3:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Anyway, what to do? I imagine we could write some code in
analyzejoins.c to consider whether ECs give rise to base restriction
clauses that they didn't before. But that seems ugly and fragile.
More generally, there is nothing that is not ugly and fragile about
analyzejoins.c's relation removal logic, and the recent addition of
self-join elimination made that situation even worse. I think we
have a permanent maintenance gotcha there.

I feel the same way about this.

I have a modest proposal to make instead: let's nuke all that logic
from orbit. Revise analyzejoins.c so that it does join removals
working strictly on the jointree representation, which is simpler
and far more stable than any of the planner's derived data. Then,
if we successfully removed any joins, throw away all the derived
data and loop back around within query_planner() to redo
deconstruct_jointree and all the rest of it.

I prototyped this proposal to see how it would look in code, mainly
copying how remove_useless_result_rtes does the removal from the join
tree. See attached PoC. Please note that this is far from
review-ready, and it only covers outer-join removal, skipping
self-join elimination for now, and it lacks badly proper comments and
test cases.

But it does fix the reported bug. It causes one plan change in the
existing tests. I haven't looked into it, but it doesn't seem like a
blocker.

Overall, the code looks much neater and easier to maintain. I think
we should go with this proposal. (I haven't benchmarked planning
performance yet, though.)

- Richard

Richard Guo <guofenglinux@gmail.com> writes:

On Tue, Jul 21, 2026 at 3:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I have a modest proposal to make instead: let's nuke all that logic
from orbit. Revise analyzejoins.c so that it does join removals
working strictly on the jointree representation, which is simpler
and far more stable than any of the planner's derived data. Then,
if we successfully removed any joins, throw away all the derived
data and loop back around within query_planner() to redo
deconstruct_jointree and all the rest of it.

I prototyped this proposal to see how it would look in code, mainly
copying how remove_useless_result_rtes does the removal from the join
tree. See attached PoC. Please note that this is far from
review-ready, and it only covers outer-join removal, skipping
self-join elimination for now, and it lacks badly proper comments and
test cases.

I spent some time hacking on this with the aid of Claude Code, and
arrived at what seems a complete patch. It's a net deletion of
over 900 lines of code, and the newly-added code is mostly very
straightforward recursions over the jointree.

I had Claude do some performance testing, and the only case that got
noticeably slower was removal of multiple self-joins (about 10%
planning time slowdown for removal of 8 self-joins). I'm not super
concerned about that; it doesn't seem like such cases would be common.

What I'm pretty unclear on is whether we want to risk back-patching
this. It's a big change, and I can't honestly promise that it doesn't
bring some new bugs. Still, it fixes one known bug and very likely
some not-yet-known ones. Perhaps a reasonable choice would be to
back-patch to v18 where self-join elimination came in, because I still
have very little faith in that code.

regards, tom lane

On Sat, 25 Jul 2026 at 17:47, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Richard Guo <guofenglinux@gmail.com> writes:

On Tue, Jul 21, 2026 at 3:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I have a modest proposal to make instead: let's nuke all that logic
from orbit. Revise analyzejoins.c so that it does join removals
working strictly on the jointree representation, which is simpler
and far more stable than any of the planner's derived data. Then,
if we successfully removed any joins, throw away all the derived
data and loop back around within query_planner() to redo
deconstruct_jointree and all the rest of it.

I prototyped this proposal to see how it would look in code, mainly
copying how remove_useless_result_rtes does the removal from the join
tree. See attached PoC. Please note that this is far from
review-ready, and it only covers outer-join removal, skipping
self-join elimination for now, and it lacks badly proper comments and
test cases.

I spent some time hacking on this with the aid of Claude Code, and
arrived at what seems a complete patch. It's a net deletion of
over 900 lines of code, and the newly-added code is mostly very
straightforward recursions over the jointree.

I had Claude do some performance testing, and the only case that got
noticeably slower was removal of multiple self-joins (about 10%
planning time slowdown for removal of 8 self-joins). I'm not super
concerned about that; it doesn't seem like such cases would be common.

What I'm pretty unclear on is whether we want to risk back-patching
this. It's a big change, and I can't honestly promise that it doesn't
bring some new bugs. Still, it fixes one known bug and very likely
some not-yet-known ones. Perhaps a reasonable choice would be to
back-patch to v18 where self-join elimination came in, because I still
have very little faith in that code.

I can't get this to crash in a standard build, but with asserts, I can:

create table a (a int primary key, b int);
create table b (a int, b int);

select a2.a
from b b1
join a a1 on b1.a = a1.a
join a a2 on a2.a = a1.a and a2.b = a1.b;

server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
!?>

I got Claude to verify and review, and it returned:

Verdict on the design: the approach is right, and it does what it
claims. The bug repro is fixed, the code is far more legible than what
it replaces, and the restart loop composes better than the old
one-shot logic (e.g. a LEFT JOIN (b LEFT JOIN c ON b.j=c.j) ON a.i=b.i
now removes both joins — the inner one first, the outer one on the
next pass). But your crash is real, it's in the self-join elimination
part, and it reaches well beyond the shape you found; separately, the
SJE planning-time cost is substantially worse than the email reports.

---
1. BLOCKER — your repro confirmed: SJE leaves quals outside their jointree scope

Reproduced exactly as you wrote it, on empty, never-analyzed tables:

create table a (a int primary key, b int);
create table b (a int, b int);

select a2.a
from b b1
join a a1 on b1.a = a1.a
join a a2 on a2.a = a1.a and a2.b = a1.b;

TRAP: failed Assert("root->hasLateralRTEs"), File: "initsplan.c", Line: 2921
LOG: client backend was terminated by signal 6: Aborted

Your read is right that a standard build doesn't show it. Same source
tree built with cassert=false plans it fine, and the plan is
byte-identical to master's:

Hash Join
Hash Cond: (b1.a = a2.a)
-> Seq Scan on b b1
-> Hash
-> Seq Scan on a a2
Filter: (b IS NOT NULL)

Root cause. remove_self_join_rel() edits the jointree first, then runs
ChangeVarNodes(parse, toRemove, toKeep). Any qual that referenced
toRemove now references toKeep, which may not be within the scope of
the jointree node the qual hangs off. distribute_qual_to_rels() then
falls into the pulled-up-LATERAL postponement branch, and that
branch's entry assertion fires.

In your query specifically: a1 is removed (the lower relid is the one
dropped), which collapses JoinExpr(b b1, a a1, ON b1.a = a1.a) into
makeFromExpr([b1], [a1.a = b1.a]); the substitution then rewrites that
qual to a2.a = b1.a inside a node whose scope is only {b1}.

It's broader than the explicit-self-join shape.
reduce_unique_semijoins() now genuinely rewrites the jointree, so
after a restart SJE can act on an ex-semijoin. That means a plain
EXISTS/IN against a unique key is enough — both of these crash the
same way, and both plan fine on master:

select a1.a from b b1 join a a1 on a1.a = b1.a
where exists (select 1 from a s where s.a = a1.a);

select a1.a from a a1 join b b1 on a1.a = b1.a
where a1.a in (select s.a from a s);

set enable_self_join_elimination = off avoids all of them, confirming
the source.

There are two distinct sub-cases, which matters for sizing the fix. I
prototyped a fix for the collapse case only (hoist the quals to the
parent when the surviving subtree doesn'tcontain toKeep); it
eliminated 33 of 46 crashes but not the rest. The remainder are
JoinExprs that don't collapse but whose ON quals referenced toRemove
while toKeep lives elsewhere in the tree. So the fix needs to be
general: relocate every qual to the lowest enclosing node whose relid
set covers its post-substitution varnos. fixup_selfjoin_jointree()
already walksthe whole jointree after the substitution and looks like
the natural home. (Prototype reverted — flagging the shape, not
proposing a patch.)

The remove_self_joins_one_group() same-side-of-every-outer-join
precondition does guarantee no outer join ever has to be crossed when
hoisting, so the relocation is safe. Worth noting the header comment
on remove_rel_from_jointree() establishes that a node can't become
empty at an outer join — true, but not the invariant that actually
matters here.

Severity. As you saw, non-assert builds don't crash: the misplaced
qual is rescued by the postponement machinery and answers stay
correct. I checked that deliberately by taking 22crashing queries and
adding a dummy , LATERAL (SELECT 1) l so hasLateralRTEs is true and
the assertion is bypassed — every result matched master. So the
production symptom is "the planner relies on a mechanism it was never
meant to reach", not wrong answers. But it kills every assert-enabled
build: beta, buildfarm, and anyone's dev tree.

Frequency: 46 crashes across ~2,900 randomly generated join queries
(1.6%); zero on master. Yours is not a corner case.

---
2. SJE planning-time regression is ~2–3×, not ~10%

remove_useless_self_joins() removes at most one join per call and
forces a full re-derivation, so N self-joins cost N+1 passes.
remove_useless_joins(), by contrast, drains all removable outer joins
in a single pass. Median Planning Time from EXPLAIN (SUMMARY ON),
21–25 reps (cassert + -O1 builds, so treat the ratios as indicative
rather than absolute):

┌────────────┬──────────┬──────────┬───────┐
│ self-joins │ patched │ base │ ratio │
├────────────┼──────────┼──────────┼───────┤
│ 8 │ 0.196 ms │ 0.113 ms │ 1.7× │
├────────────┼──────────┼──────────┼───────┤
│ 12 │ 0.369 │ 0.202 │ 1.8× │
├────────────┼──────────┼──────────┼───────┤
│ 16 │ 0.567 │ 0.251 │ 2.3× │
├────────────┼──────────┼──────────┼───────┤
│ 20 │ 0.903 │ 0.381 │ 2.4× │
├────────────┼──────────┼──────────┼───────┤
│ 24 │ 1.375 │ 0.465 │ 3.0× │
├────────────┼──────────┼──────────┼───────┤
│ 28 │ 1.705 │ 0.621 │ 2.7× │
└────────────┴──────────┴──────────┴───────┘

Outer-join removal actually got faster (16-way chain: 0.158 vs 0.201
ms), and queries with nothing to remove are unchanged. The cost is
specifically the one-removal-per-restart policyin SJE. Worth
re-checking the methodology behind the "about 10% for 8 self-joins"
figure, and worth considering letting remove_self_joins_one_group()
drain all independent pairs before returning — the same staleness
argument remove_useless_joins() makes should apply.

----------------

Thom

Thom Brown <thom@linux.com> writes:

I can't get this to crash in a standard build, but with asserts, I can:

Thanks for the report! I'd seen the same failed
Assert("root->hasLateralRTEs") in the core tests with an earlier draft
of this patch, but I thought it was resolved. Obviously not ... I'll
look closer tomorrow.

As for the speed question, I'd not poked at the details of Claude's
claim, but I guess I should have.

regards, tom lane

Here's a patchset addressing that. 0001-0003 are the same as before,
and then 0004 fixes this bug and adds tests.

Your Claude session mentioned a different failure scenario, but
gave no examples so I'm not quite sure if this covers it or not.

regards, tom lane

These patches fix all my known failure cases (attached.) In addition, as a
plus, a small set of queries that previously didn't compile are now
accepted, for example:

SELECT coalesce(u1.id + 1, 1) AS o0
FROM (u AS u1 FULL JOIN (u AS u2 JOIN u AS u3 ON u2.id = u3.id)
ON u2.id = u2.id)
GROUP BY 1;

... previously gave ERROR: FULL JOIN is only supported with merge-joinable
or hash-joinable join conditions

now returns 10 rows (`2 … 11`), which I believe is correct.

```
HashAggregate
Group Key: COALESCE((u1.id + 1), 1)
-> Merge Full Join
-> Seq Scan on u u1
-> Materialize
-> Seq Scan on u u3
```

So that's nice.

On Sat, Jul 25, 2026 at 7:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

...For completeness the definition of u here is like so:

CREATE TABLE u (id int PRIMARY KEY, a int);
INSERT INTO u (id, a) VALUES
(1, 1),
(2, 2),
(3, 3),
(4, 0),
(5, 1),
(6, 2),
(7, 3),
(8, 0),
(9, 1),
(10, 2);

On Sun, 26 Jul 2026 at 03:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Here's a patchset addressing that. 0001-0003 are the same as before,
and then 0004 fixes this bug and adds tests.

Your Claude session mentioned a different failure scenario, but
gave no examples so I'm not quite sure if this covers it or not.

Thanks. I've re-tested against these and I'm no longer able to crash it.

The different failure scenario was brought up when Claude also tested
the latest patches:

<claude>

My evidence for a second case was that I'd prototyped a fix covering
only the collapsed-node path and it killed 33 of 46 fuzz crashes,
leaving 13. I described the surviving class but never reduced one,
which wasn't much use to anyone. It is:

a JoinExpr that does not collapse, but whose ON qual referenced the
removed relation, while the kept relation lives outside that JoinExpr.

create table a (a int primary key, b int);
create table b (a int, b int);
create table c (a int, b int);

select a2.a
from ((b b1 join a a1 on true) join c c1 on c1.a = a1.a)
join a a2 on a2.a = a1.a and a2.b = a1.b;

On v2 this trips the same Assert("root->hasLateralRTEs"). Here a1 is
removed and a2 kept. The inner b1 join a1 on true collapses to b1 —
that's the case your v2 code already reasoned about. But the middle
JoinExpr keeps both children (b1 and c1), so nothing collapses there;
its ON qual is simply rewritten from c1.a = a1.a to c1.a = a2.a, and
a2 isn't in that JoinExpr's scope. A fix that only inspects nodes
which lost a child can't see it.

v3 handles it, because you keyed the hoist off "mentions relid" rather
than off collapse:

Nested Loop
-> Seq Scan on b b1
-> Materialize
-> Hash Join
Inner Unique: true
Hash Cond: (c1.a = a2.a)
-> Seq Scan on c c1
-> Hash
-> Seq Scan on a a2
Filter: (a2.b IS NOT NULL)

Your two new regression tests both exercise the collapse path (the
second via semijoin reduction). Something like the above would be
worth adding as a third, since it's the case a narrower fix would have
missed — it needs a second non-self-joined table, so it doesn't drop
straight into the sj-only section.

</claude>

And it still reports the same previous planner regression, although
thinks it may be considered acceptable. It also mentioned adding
jointype assertions because the hoisting code doesn't have any.

But all good as far as I can see.

Thom

Thom Brown <thom@linux.com> writes:

Thanks. I've re-tested against these and I'm no longer able to crash it.

Thanks for testing!

And it still reports the same previous planner regression, although
thinks it may be considered acceptable. It also mentioned adding
jointype assertions because the hoisting code doesn't have any.

Yeah, it occurred to me right after sending out the v3 patchset that
fixup_selfjoin_jointree should have an Assert that it doesn't hoist
anything above an outer join. Revised 0004 attached has that, and
adds a test case based on this example. 0001-0003 still the same.

As for the performance issue, I attach an 0005 that changes the
self-join logic to perform as many removals as it can. I don't
have a huge amount of faith in that, but I can't immediately see
any reason why it's not okay. It seems almost certainly okay to
perform SJE within groups of rels with different common OIDs, and
even within a single group, an earlier removal doesn't look like
it'd trash anything we are looking at. But it'd be worth running
a fuzzer to see if any cases turn up where the patchset fails or
gives different answers with/without 0005.

regards, tom lane

#text/x-diff; name="v4-0001-Postpone-initialization-of-all_result_relids-leaf.patch" [v4-0001-Postpone-initialization-of-all_result_relids-leaf.patch] /home/tgl/pgsql/v4-0001-Postpone-initialization-of-all_result_relids-leaf.patch
#text/x-diff; name="v4-0002-Perform-join-removal-by-editing-the-query-s-joint.patch" [v4-0002-Perform-join-removal-by-editing-the-query-s-joint.patch] /home/tgl/pgsql/v4-0002-Perform-join-removal-by-editing-the-query-s-joint.patch
#text/x-diff; name="v4-0003-Remove-dead-code-that-s-no-longer-needed-for-join.patch" [v4-0003-Remove-dead-code-that-s-no-longer-needed-for-join.patch] /home/tgl/pgsql/v4-0003-Remove-dead-code-that-s-no-longer-needed-for-join.patch
#text/x-diff; name="v4-0004-Ensure-we-hoist-modified-quals-to-the-right-join-.patch" [v4-0004-Ensure-we-hoist-modified-quals-to-the-right-join-.patch] /home/tgl/pgsql/v4-0004-Ensure-we-hoist-modified-quals-to-the-right-join-.patch
#text/x-diff; name="v4-0005-Allow-remove_useless_self_joins-to-remove-multipl.patch" [v4-0005-Allow-remove_useless_self_joins-to-remove-multipl.patch] /home/tgl/pgsql/v4-0005-Allow-remove_useless_self_joins-to-remove-multipl.patch

Drat, failed to hit control-C control-E. Now with patches attached,
for sure.

regards, tom lane

I wrote:

As for the speed question, I'd not poked at the details of Claude's
claim, but I guess I should have.

I asked Claude to explain exactly how it arrived at its numbers,
and the answer is that it missed the problem the first time, partly
through not trying to go higher than N=8. Full details attached for
the archives' sake, though I think this is now of mostly academic
interest.

regards, tom lane

On Mon, Jul 27, 2026 at 2:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Drat, failed to hit control-C control-E. Now with patches attached,
for sure.

Thanks for the patchset. Nice cleanup:

911 insertions(+), 1670 deletions(-)

I noticed that the call to remove_rels_from_query_tree() at the end of
remove_useless_joins() performs a full traversal over the query tree
to delete the removed relids. I wonder if this is a bit too
brute-force, as AFAICS only PHVs can still contain references to the
removed relids. If there were any other remaining references,
join_is_removable() wouldn't have considered the join removable in the
first place.

If that's correct, perhaps we can simplify
remove_rels_from_query_tree() to something lighter, similar to
remove_result_refs().

Also, the issue with duplicate clauses after self-join elimination
reported in bug #19435 is still present in v4. It would be great if
we could address that in passing as well.

Here is a simplified repro:

create table t (a int unique, b int);

explain (costs off)
select * from t t1 left join
(t t2 join t t3 on t2.a = t3.a) on t3.a is not null;
QUERY PLAN
-------------------------------------------------------------
Nested Loop Left Join
-> Seq Scan on t t1
-> Materialize
-> Seq Scan on t t3
Filter: ((a IS NOT NULL) AND (a IS NOT NULL))
(5 rows)

- Richard

Richard Guo <guofenglinux@gmail.com> writes:

Thanks for the patchset. Nice cleanup:

Thanks for looking at it!

I noticed that the call to remove_rels_from_query_tree() at the end of
remove_useless_joins() performs a full traversal over the query tree
to delete the removed relids. I wonder if this is a bit too
brute-force, as AFAICS only PHVs can still contain references to the
removed relids. If there were any other remaining references,
join_is_removable() wouldn't have considered the join removable in the
first place.

Yeah, I think this is probably true. The Claude-written comment
claims that the removed OJ's relid could appear in surviving Vars'
nullingrels, but I don't see how that could be: such Vars would
have to refer to the removed baserel, and then they should have
prevented the join removal.

If that's correct, perhaps we can simplify
remove_rels_from_query_tree() to something lighter, similar to
remove_result_refs().

Meh. I don't think there's a lot to be gained there: we still
have to walk the whole tree, and I'm not excited about duplicating
tree-walking code. We could maybe use substitute_phv_relids from
prepjointree.c, but I see that has Assert(!IsA(node, AppendRelInfo));
which we'd have to drop. I doubt there'd be any measurable
improvement over the ChangeVarNodes coding, anyway.

Also, the issue with duplicate clauses after self-join elimination
reported in bug #19435 is still present in v4. It would be great if
we could address that in passing as well.

I'm not excited about that either. AFAICS, with the #19435 query:

SELECT 1 AS c1
FROM (
pg_table_a AS tom0
RIGHT JOIN (
(pg_table_a AS tom1 NATURAL JOIN pg_table_a AS tom2)
RIGHT JOIN pg_table_a AS tom3
ON tom1.col_bool IS NOT NULL
)
ON tom1.col_bool
);

The NATURAL JOIN produces quals

tom1.id = tom2.id AND tom1.col_bool = tom2.col_bool

SJE correctly replaces those with

tom2.id IS NOT NULL AND tom2.col_bool IS NOT NULL

We do, later on, detect that the id clause is unnecessary because of
the id column's NOT NULL constraint. But we don't notice that the
generated col_bool clause is redundant with the user-written
"tom1.col_bool IS NOT NULL" clause appearing in a higher join level.
I don't think it's incumbent on us to fix redundantly-written
user queries, and even if we took that on, the SJE code would not
be the place for it. You can get similar redundancies without
any SJE happening, eg

regression=# explain (costs off)
select * from pg_table_a as tom1 left join
(select * from pg_table_a as tom2 where tom2.col_bool is not null) tom3
on tom3.col_bool is not null;
QUERY PLAN
---------------------------------------------------------------------------
Nested Loop Left Join
-> Seq Scan on pg_table_a tom1
-> Materialize
-> Seq Scan on pg_table_a tom2
Filter: ((col_bool IS NOT NULL) AND (col_bool IS NOT NULL))
(5 rows)

Here is a simplified repro:
create table t (a int unique, b int);
explain (costs off)
select * from t t1 left join
(t t2 join t t3 on t2.a = t3.a) on t3.a is not null;

On what grounds are these not just stupidly-written queries?
We shouldn't fail of course (which was the actual complaint in
#19435), but I'm not excited about removing the user's redundancy.

If somebody held a gun to my head and said "fix that", I'd probably
teach distribute_quals_to_rels to de-duplicate quals as they go into
baserestrictinfo and joinrestrictinfo lists. But I'm quite certain
that the planner cycles spent on that would be a net loss for most
people.

regards, tom lane

I spent some time looking into what would be involved in back-patching
this. Quite aside from risks of new bugs, there's an API/ABI break
involved for released branches (I don't think we are worried about
that yet for v19, though). One thing we can do to reduce the API risk
is to not delete the no-longer-used extern functions that the master
patch removes, such as add_vars_to_attr_needed. I kind of doubt that
anyone is using those in extensions, but leaving them as dead code
seems like cheap insurance. However, there's an irreducible minimum
API change in remove_useless_joins, reduce_unique_semijoins, and
remove_useless_self_joins: those don't return what they did before,
and even more importantly their side effects are much different from
before. I don't see any evidence in Debian Code Search that anyone is
calling those, so I'm not inclined to go to lengths like (say) keeping
the whole of the existing analyzejoins.c code around as dead code.
Even if we wanted to consider that, the bugs that we're trying to get
rid of would still bite any hypothetical extension that was messing
with these functions. So I think it's reasonable to trust that nobody
is calling anything lower-level than query_planner().

Having said that, I think that it'd be a good idea to rename
remove_useless_joins to remove_useless_outer_joins. The existing
name is already confusing now that we have remove_useless_self_joins
too. The ABI angle is that if we do that, then any extension that's
trying to call remove_useless_joins will fail with an
easily-understood error at library load time, rather than crashing
or misbehaving in some arcane way because the function didn't do what
it expected. We could extend this to renaming all three functions,
but I think renaming remove_useless_joins is sufficient --- I really
don't see any use case for calling the other two and not that one.

So attached are a set of patches along that line. v5-0001 is the
master patchset squashed to one patch; it differs from the v4 series
in having the remove_useless_joins rename and some minor additional
comment-smithing. The others are draft patches for v16-v19. The
v19 patch is only trivially different from master. v18 and earlier
don't remove any of the dead code outside analyzejoins.c. v16/v17
are a good deal smaller because there's no SJE code to fix.

I think it's still open to debate how far back we want to go.
In principle we could make these changes in v14/v15, but with no known
bugs manifesting in those branches, the risk/reward ratio seems poor,
so I didn't make patches for those. I think we definitely want to do
this in v18, since aside from #19560 that branch has the SJE bug that
Jacob B. reported at [1]/messages/by-id/CA+COZaBVFHS-eXL7a53iZfNJyv5LqD5t1eSbYDavYxLSyfkL6A@mail.gmail.com (which is a distinct problem, per Alexander's
analysis). v16 and v17 seem like a gray area, but we do know that
they have #19560. Even if we limit our ambition in those branches
to fixing that problem, I don't see a simple localized fix for it.

Thoughts?

regards, tom lane

[1]: /messages/by-id/CA+COZaBVFHS-eXL7a53iZfNJyv5LqD5t1eSbYDavYxLSyfkL6A@mail.gmail.com

Tom Lane <tgl@sss.pgh.pa.us> 于2026年7月28日周二 02:21写道：

I think it's still open to debate how far back we want to go.
In principle we could make these changes in v14/v15, but with no known
bugs manifesting in those branches, the risk/reward ratio seems poor,
so I didn't make patches for those. I think we definitely want to do
this in v18, since aside from #19560 that branch has the SJE bug that
Jacob B. reported at [1] (which is a distinct problem, per Alexander's
analysis). v16 and v17 seem like a gray area, but we do know that
they have #19560. Even if we limit our ambition in those branches
to fixing that problem, I don't see a simple localized fix for it.

Thoughts?

My question is unrelated to back-patching, so apologies if this is not
the right thread.

When I first saw your proposal to rewrite analyzejoins.c, I wondered
whether it might also address the remaining SJE issue discussed in
[1]: /messages/by-id/CAHewXN=7kDJjUcgEm+6qhaKOXuqzvhRqAAKdafNCRgn0yH7BGg@mail.gmail.com

However, after applying v5 on top of HEAD, the issue still seems to be
present. In particular, the resulting plan retains a redundant filter
after SJE:

CREATE TABLE pg_table_a (
id INTEGER PRIMARY KEY,
col_bool BOOLEAN
);

INSERT INTO pg_table_a (id, col_bool)
VALUES (5, TRUE);

EXPLAIN
SELECT 1 AS c1
FROM (
pg_table_a AS tom0
RIGHT JOIN (
(pg_table_a AS tom1 NATURAL JOIN pg_table_a AS tom2)
RIGHT JOIN pg_table_a AS tom3
ON tom1.col_bool IS NOT NULL
)
ON tom1.col_bool
);

The relevant part of the plan is:

Seq Scan on pg_table_a tom2
Filter: ((col_bool IS NOT NULL) AND (col_bool IS NOT NULL))

That duplicate filter appears to be left behind after the join
involving tom1 is removed.

I think this issue is still worth fixing. The discussion in [1]/messages/by-id/CAHewXN=7kDJjUcgEm+6qhaKOXuqzvhRqAAKdafNCRgn0yH7BGg@mail.gmail.com has
been inactive for about two months, so I wanted to ask whether you
intend to address it as part of the analyzejoins.c rewrite, or whether
it would be better for me to resume work on [1]/messages/by-id/CAHewXN=7kDJjUcgEm+6qhaKOXuqzvhRqAAKdafNCRgn0yH7BGg@mail.gmail.com once the rewrite has
been committed.

[1]: /messages/by-id/CAHewXN=7kDJjUcgEm+6qhaKOXuqzvhRqAAKdafNCRgn0yH7BGg@mail.gmail.com

--
Thanks,
Tender Wang

Hi Tom,

Tom Lane <tgl@sss.pgh.pa.us> 于2026年7月28日周二 20:24写道：

Tender Wang <tndrwang@gmail.com> writes:

I think this issue is still worth fixing. The discussion in [1] has
been inactive for about two months, so I wanted to ask whether you
intend to address it as part of the analyzejoins.c rewrite, or whether
it would be better for me to resume work on [1] once the rewrite has
been committed.

I disagree. That's not a bug -- okay, it's a missed optimization for
a small class of poorly-written queries -- and it's not specific to
SJE either. I think far too much development effort has been spent
on the point already. See my upthread reply to Richard [1].

Thanks for the explanation.

I'm not sure why I couldn't see your reply to Richard in my Gmail
inbox, although I can see it on the pgsql-hackers archive.

Anyway, thanks for the clarification, and sorry for raising a point
that had already been discussed.

--
Thanks,
Tender Wang

On Tue, Jul 28, 2026 at 3:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

So attached are a set of patches along that line. v5-0001 is the
master patchset squashed to one patch; it differs from the v4 series
in having the remove_useless_joins rename and some minor additional
comment-smithing.

I reviewed v5-0001, and here are some comments.

1. The initialization of all_result_relids/leaf_result_relids is moved
to below the trivial-jointree early return. So for a query like
"INSERT INTO t VALUES()", both sets now stay NULL where they used to
be {resultRelation}.

I'm not sure if this can cause any issues, but at least it contradicts
the comment in pathnodes.h:

* all_result_relids is empty for SELECT, otherwise it contains at least
* parse->resultRelation.

2. Regarding remove_rels_from_query_tree(): I complained about this
function in my previous email, but it does not get changed.

First, I think we can at least add a check on root->glob->lastPHId and
do the ChangeVarNodes walks only if there are any PHVs in the query.
I believe only PHVs can still contain references to the removed relids
here.

Second, as you mentioned, the comment claims that "the relids can
still appear in the nullingrels sets of surviving Vars", but that is
not true. We should not keep such comment as it is very misleading.

Third, this function works only if "There should be no ordinary Vars
of a removed relation left". I agree it holds today. But if it ever
does break, ChangeVarNodes sets var->varno = -1, which is INNER_VAR,
and AppendRelInfo->parent_relid/child_relid would go to -1 too. This
would produce garbage rather than an error, which makes me nervous.
I hope we can use a walker that mutates on PHVs only.

3. In reduce_unique_semijoins(), analyzejoins.c:480 tests
min_righthand for singleton-ness, but analyzejoins.c:517 uses
syn_righthand to find the JoinExpr and flips the whole node to
JOIN_INNER. If there is a case where min_righthand is singleton but
syn_righthand is not and the innerrel is unique, we will be in
trouble.

4. The commit message claims that "it doesn't seem to result in any
significant planning-time penalty". I'm somewhat skeptical of that.

- Richard