Re: remove_useless_joins vs. bug #19560

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> On Mon, Aug 17, 2026 at 8:58 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> One idea for fixing this is to null out the subquery field of a
>> subquery RTE as soon as we've pulled it up, but there is probably
>> code that will crash on a null subquery pointer.

> It seems to me that the subquery in question isn't a pulled-up one.
> pull_up_simple_subquery() already sets rte->subquery to NULL at line
> prepjointree.c:1725.
> What is in question is the subquery of a rel removed by join removal
> itself. Its RTE stays in the rangetable with the query tree intact,
> and that tree can contain lateral references to rels that are still
> around.

Hah, thanks for identifying the locus of that issue.

I still don't like replacing this usage of ChangeVarNodes with
substitute_phv_relids though. While it's a kluge as-presented,
I'm not sure that using substitute_phv_relids that way is much less
of a kluge. Moreover, if there were any leftover Vars in the tree,
substitute_phv_relids wouldn't notice. I think noticing that is a
good thing, and the fact that it identified this oversight in
remove_useless_outer_joins seems like evidence in favor of that.

So what I'd prefer to do is as in 0002 attached. For simplicity of
review, 0001 is the same as the v5 combined patch I presented before.
(But it's rebased onto today's HEAD, which changes only line numbers,
and I tweaked the commit message's claim about planning time per your
other comments.) 0002 changes ChangeVarNodes' API to have a clean
way to represent "we're removing this relid", and adds the fix you
identified with a more extensive comment.

(Maybe it'd make sense to use this version of ChangeVarNodes in
prepjointree too? I didn't look into that.)

regards, tom lane