Re: remove_useless_joins vs. bug #19560

On Fri, Aug 28, 2026 at 12:53 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I don't like this on the grounds of consistency. As things stand now,
> we partially initialize those fields in subquery_planner and then
> finish filling them in much later, during add_other_rels_to_query.
> What I did in my proposed patch was to collect all of that processing
> together, in successive steps near the end of query_planner. This
> makes it clear that those fields are not to be relied on before that
> point, and in particular that they don't need to be reset in the
> restart loop in the initial part of query_planner. If we change it
> back as you suggest, then it'll be really nonobvious why this derived
> data is handled differently from other derived data. And the fact
> that it's only partially valid during the restart loop seems to me
> to be a bug hazard: if someone tried to rely on these fields in
> logic within that loop, it might seem to work as long as they'd not
> tested it on inheritance/partitioning cases. So I think moving this
> logic is good cleanup, albeit maybe not strictly necessary to the
> immediate problem.
>
> I take your point that the early-exit-for-trivial-jointree path now
> fails to set these fields at all, but I'd rather handle that by
> adding a couple more lines in that path to fill them in. Yup, it'd
> be duplicate logic, but it seems cleaner that way.

Fair enough. I agree that keeping all of this together at the end of
query_planner makes it clear that these fields are not valid before
then, and that a half-filled set during the restart loop is a hazard.
Adding the few lines to the early-exit path sounds good to me.

> Well, yeah, but on that argument we could drop the entire stanza,
> because it's just checking that join_is_removable didn't mess up.
> Maybe we should? I've not heard that anybody ever hit those Asserts.

I think so. The resultRelation case is rejected explicitly in
join_is_removable:

if (innerrelid == root->parse->resultRelation)
return false;

and a row-marked rel is rejected by the attr_needed test, since
preprocess_targetlist adds junk columns for every row mark.

I'm not a fan of redundant Asserts, so I'd vote for dropping both.
But I'm fine with keeping both too, if you prefer.

- Richard