Re: remove_useless_joins vs. bug #19560

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> According to that, I can reproduce this same issue with the query
> below:

> create table t (a int primary key);

> select 1 from t t1 left join
> (t t2 left join t t3 on t3.a = t2.a) on true
> where t2.a is null and t3.a is null;

> 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.

> With the quals removed before deconstruct_jointree,
> check_redundant_nullability_qual has nothing left to do, so I think we
> can get rid of it in passing.

Right, what this is really doing is moving that processing out of
the deconstruct_jointree phase and requiring reduce_outer_joins to
clean up its own mess. Our git history shows I'm to blame for the
way it's done now, but I sure don't remember why I did it like that.
This way seems clearly neater: fewer moving parts, less inconsistency
between different representations of the query.

A few nits, mostly about commentary:

* 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.

* 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.

* 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.

* 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 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? (Maybe you are comparing this to find_forced_null_vars,
but the reason that handles both Lists and not-Lists is that it
processes a List via self-recursion. This doesn't, so I think it
could just be a simple foreach loop over the argument. Maybe use
castNode(List, quals) to enforce the assumption.)

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.

regards, tom lane