Re: Useless LEFT JOIN breaks MIN/MAX optimization

Hi Alena,

If I understand correctly, the problem here is that join removal and
minmax aggregates don't work well together: after join removal runs,
we end up with a state that doesn't permit the minmax-aggregate code
to work.

I agree that would be good to fix but the patch doesn't seem right to me.

I'm looking at this line from the patch, in particular:

+ if (IsA(jtnode, JoinExpr) && jtnode->fromlist == NIL &&
IsA(jtnode->quals, RangeTblRef))

jtnode is declared to be of type FromExpr. But here you check that it
is a JoinExpr, and if it is, then you dereference it as if it were a
FromExpr. So, if I'm understanding correctly, the reference to
jtnode->fromlist is actually looking at some completely unrelated
field that is part of a JoinExpr, like maybe larg, and jtnode->quals
is looking at some other field, maybe jtnode->rarg. That's pretty
crazy coding -- the right thing to do would be to cast the jtnode to
the proper type and then access it using the proper field names.

Backing up a step, it seems to me that it might be a good idea to
rewrite the preceding loop, which can descend through any number of
levels of FromExpr expressions, to be able to descend through either
FromExpr or JoinExpr expressions, stopping when it can't descend
further using either method. The way you've coded it supposes that it
only ever needs to descend through one JoinExpr expression and that
the JoinExpr will always be beneath all the FromExprs, which might not
be a correct assumption. It would probably be a good idea to write
some more complex test cases where multiple joins get removed at
various levels, and where there are also subquery levels that produce
FromExprs, in various configurations, and test that the patch works in
all of those cases.

But that's also assuming that you're correct here about how to descend
through a JoinExpr, which I'm not quite sure whether is true. It's
also assuming that we should solve the problem here rather than in
some other part of the code e.g. the join removal code, and I'm not
sure about that either.

I'm just studying this for the first time so apologies if this review
is not quite up to standard.

Thanks,

--
Robert Haas
EDB: http://www.enterprisedb.com