Re: Useless LEFT JOIN breaks MIN/MAX optimization

Hi, Robert!

On 09.05.2025 20:12, Robert Haas wrote:
> 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.
Yes, it is correct.
> 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.
Yes, you are right, my mistake. I'll correct it. Thank you)
> 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.

You are right, there are not enough tests here and we need to add
queries with more complex semantics and you are right about the approach.

I'll implement it. Thanks for pointing this out, I missed it.

> 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 don’t think it’s necessary to move this code into the join removal
optimization phase. There’s no guarantee that there will not appear
future optimizations that will make impossible to apply min/max
optimization afterward. Keeping the min/max optimization in a single,
consistent location also improves clarity and maintainability of the code.

The simplest solution, in my opinion, is to extend the current logic to
include type checking for the JoinExpr types as I did in my patch.
Specifically, we can verify that it now involves only a single relation
or partitioned relations without formation join algorithms.

> I'm just studying this for the first time so apologies if this review
> is not quite up to standard.
No need to apologize - you raised important points during both the patch
and issue reviews. Thanks for your valuable and helpful feedback!

--
Regards,
Alena Rybakina
Postgres Professional