Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> OK, I'm marking this ready for committer attention, on the
> understanding that that doesn't include the invtrans_minmax patch.
I've finished reviewing/revising/committing this submission.
Some notes:
* I left out the EXPLAIN ANALYZE statistics, as I still feel that they're
of little if any use to regular users, and neither very well defined nor
well documented. We can revisit that later of course.
* I also left out the table documenting which aggregates have this
optimization. That's not the kind of thing we ordinarily document,
and it seems inevitable to me that such a table would be noteworthy
mostly for wrong/incomplete/obsolete information in the future.
* I rejected the invtrans_collecting sub-patch altogether. I didn't
like anything about the idea of adding a poorly-documented field to
ArrayBuildState and then teaching some random subset of the functions
using that struct what to do with it. I think it would've been simpler,
more reliable, and not that much less efficient to just do memmove's in
shiftArrayResult. The string-aggregate changes were at least more
localized, but they still seemed awfully messy. And there's a bigger
issue: these aggregates have to do O(N) work per row for a frame of length
N anyway, so it's not clear to me that there's any big-O gain to be had
from making them into moving aggregates. I doubt people are going to be
using these aggregates with very wide frames, just because they're going
to be horribly expensive no matter what we do.
Anyway, this is nice forward progress for 9.4, even if we get no further.
regards, tom lane