Re: Expand applicability of aggregate's sortop optimization

Andrei Lepikhov <lepihov@gmail.com> writes:
> Rebase on current master.

I started to look at v5, and was immediately put off by the fact
that its documentation of the new support request type consists of
precisely zero words.  You have the wrong idea of what is required
when adding a support request.  The specification of what the request
does and what it returns is your PRIMARY work product, not something
you can just blow off and expect people to reverse-engineer over and
over again in the future.  Please look at the existing entries in
supportnodes.h and note that they all have quite specific and detailed
comments.

minmax_support() is pretty short on comments as well.  The one comment
that's there is unintelligible because it starts out by talking about
an index ... the reader is likely to say "what index?"  Also, I am
far from convinced that the code is right: the fact that the aggsortop
shares a btree opfamily with orderClause->sortop doesn't seem to be
enough to justify removing the aggorder clause.  What if they are
opposite sort directions?

Actually ... even if those operators do match, is it okay to just
remove the aggorder clause?  What happens if we do not end up using
an index-based implementation?  The code really needs to include 
a defense of why it's okay to do what it's doing.

Also, that one comment says "So, we can still use the index IFF the
aggregated expression equals the expression used in the ordering
operation".  But I see no check that they match.

I kind of wonder whether the case that this is attempting to optimize
actually occurs in the field often enough to justify expending this
many cycles looking for it.  MIN(x ORDER BY x) does not seem like
something that a rational person would write.

BTW, it looks to me like that Assert would fail on MIN(x ORDER BY x,y)
which is even less sensible to write, but that doesn't make an
assertion failure okay.

I wonder whether you picked the best spot to insert the hook call
in preprocess_aggref.  In particular, the hook can do nothing that
would affect the polymorphic-result-type resolution step, which
is maybe not a restriction in practice but I'm not entirely sure.
I'm a little inclined to call it before we start digging information
out of the Aggref, rather than after.

            regards, tom lane