Re: remove_useless_joins vs. bug #19560

On Tue, Sep 1, 2026 at 12:56 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> > I think the real problem is that we leave a qual in the tree after
> > deciding that it is redundant. So the attached patch removes such
> > quals from the jointree at the end of reduce_outer_joins.

> +1 for concept.

Thanks for reviewing this!

> * Your commit message explains this as
> "check_redundant_nullability_qual has nothing left to detect, so
> remove it". But as I just said, it's better to think of this change
> as moving this processing to an earlier phase, so that should be
> explained differently.

Agreed. I've reworded the commit message to explain it that way.

> * The comment for find_forced_null_var says it's used by initsplan.c.
> But what that is referencing is the call from
> check_redundant_nullability_qual; the usage is moving to
> prepjointree.c, so that comment needs a small adjustment.

Fixed.

> * The new comment for remove_redundant_nullability_quals_recurse says
> "they would retain references to rels that join removal might later
> delete", but I don't think that's quite accurate. If we didn't do
> this qual-removal in either phase, then the Var reference would still
> be there and join removal would not think it can remove the join.
> So it seems to me that better wording would be like "they could
> prevent join removal". But that puts the lie to the claim that this
> is "not just an optimization", so the whole sentence needs a bit of
> reworking.

Right. I've reworked that sentence to say that "Keeping such a qual
would not be wrong, but it would generate bogus selectivity estimates,
and it could prevent join removal from later removing the rel(s) it
references."

> * I feel like the naming of remove_redundant_nullability_quals
> and remove_redundant_nullability_quals_recurse is a bit weird.
> Usually our pattern is that a function named like foo_recurse
> is an inner workhorse for a function named foo. This is the
> other way around, which seems like it'd confuse readers.

I've renamed the jointree walker to remove_redundant_nullability_quals
(it recurses into itself, so there is no wrapper to pair a "_recurse"
with), and the helper that processes one qual list to
strip_redundant_nullability_quals. Not sure if this is the best
naming though.

> * I don't understand the coding in remove_redundant_nullability_quals.
> We require quals to be in implicit-AND format by this point, so they
> should be either NIL or a List. Is the last stanza actually
> reachable?

Good catch. You're right, it is not reachable. It's now a simple
loop over castNode(List, quals), as you suggested.

> What I'm taking away from all this is that 2ebf25e7d is forcing us
> to be neater about management of the parse tree, which is probably
> a good thing overall.

Agreed. Before 2ebf25e7d, there were quite a few cases where an
expression was removed logically but still present in the parse tree
physically, and nothing ever walked the tree afterwards expecting
consistency. Now the new Asserts are shaking out all these cases, and
in the end we'll get a much neater parse tree.

- Richard