On 27 June 2016 at 03:36, Tom Lane <t...@sss.pgh.pa.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
aggtest.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
