Re: Rethinking representation of partial-aggregate steps

On 27 June 2016 at 03:36, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> After having worked on the patch some, I think that AggSplit is a pretty
> good choice, because it is about how we split up the calculation of an
> aggregate. And it's short, which is a good thing for something that's
> going to be a component of assorted names.

I wish to thank you for working hard to improve this area of the code.
I had a read over your changes and I think there's quite a large
problem with your code which realy needs some more thinking:

I can't help wonder how plan to allow future expansions of
non-serialized partial aggregates giving that in setrefs.c you're
making a hard assumption that mark_partial_aggref() should always
receive the SERIAL versions of the aggsplit. This outright wrong as
the Agg node may not be a serialized aggregate node at all.

The attached very rough patch hacks together some code which has the
planner generate some partial aggregates which are not serialized.
Here's what I get:

CREATE AGGREGATE myavg (numeric)
(
stype = internal,
sfunc = numeric_avg_accum,
finalfunc = numeric_avg,
combinefunc = numeric_avg_combine
);
create table a (num numeric not null);
insert into a select generate_series(1,10);
select myavg(num) from a;
ERROR: invalid memory alloc request size 18446744072025250716

This is down to the aggtype being set to BYTEA regardless of the fact
that it's not a serialized aggregate.

Passing the non-serial versions of the enum to mark_partial_aggref()
makes this case work, but that's certainly not the fix.

David

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services