Florian Pflug <fgp@phlo.org> writes:
> I was (and still am) not in favour of duplicating the whole quadruple of
> (state, initialvalue, transferfunction, finalfunction) because it seems
> excessive. In fact, I believed that doing this would probably be grounds for
> outright rejection of the patch, on the base of catalog bloat. And your
> initial response to this suggestion seemed to confirm this.
Well, I think it's much more likely that causing a performance penalty for
cases unrelated to window aggregates would lead to outright rejection :-(.
The majority of our users probably don't ever use window functions, but
for sure they've heard of SUM(). We can't penalize the non-window case.
Expanding pg_aggregate from 10 columns (as per patch) to 14 (as per this
suggestion) is a little annoying but it doesn't sound like a show stopper.
It seems reasonable to assume that the extra initval would be NULL in most
cases, so it's probably a net addition of 12 bytes per row.
> On Apr9, 2014, at 20:20 , Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The patch has in fact already done that to a couple of basic aggregates like
>> sum(int4). Has anyone bothered to test what side-effects that has on
>> non-windowed aggregation performance?
> I'm pretty sure David Rowley did some benchmarking. The results should be
> in this thread somewhere I think, but they currently evade me... Maybe David
> can re-post, if he's following this...
I saw benchmarks addressing window aggregation, but none looking for
side-effects on plain aggregation.
> If we really go down that road (and I'm far from convinced), then maybe
> instead of having a bunch of additional fields, we could have separate
> entries in pg_aggregate for the two cases, with links between them.
That seems like a complete mess; in particular it would break the primary
key for pg_aggregate (aggfnoid), and probably break every existing query
that looks at pg_aggregate. Some extra fields would not break such
expectations (in fact we've added fields to pg_aggregate in the past).
regards, tom lane