Re: remove_useless_joins vs. bug #19560

On Sat, 25 Jul 2026 at 17:47, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> > On Tue, Jul 21, 2026 at 3:52 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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