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