Re: Combining Aggregates

On 18 March 2016 at 13:39, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> wrote:
> On Thu, Mar 17, 2016 at 10:59 PM, Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>> Hi,
>>
>> On 03/17/2016 12:53 PM, David Rowley wrote:
>>>
>> ...
>>>
>>>
>>> I just had a quick skim over the patch and noticed the naming
>>> convention you're using for the combine function is *_pl, and you have
>>> float8_pl. There's already a function named float8pl() which is quite
>>> close to what you have. I've been sticking to *_combine() for these,
>>> so maybe float8_combine() and float8_regr_combine() are better names.
>>
>>
>> +1 to the _combine naming convention.
>
> Thanks for the input. Makes sense, updated patch is attached with
> the changes.

I've had a look over this. I had to first base it on the 0005 patch,
as it seemed like the pg_aggregate.h changes didn't include the
serialfn and deserialfn changes, and an OID was being consumed by
another function I added in patch 0003.

On testing I also noticed some wrong results, which on investigation,
are due to the wrong array elements being added together.

For example:

postgres=# select stddev(num) from f;
stddev
------------------
28867.5149028984
(1 row)

postgres=# set max_parallel_degree=8;
SET
postgres=# select stddev(num) from f;
stddev
--------
0
(1 row)

+ N += transvalues2[0];
+ sumX += transvalues2[1];
+ CHECKFLOATVAL(sumX, isinf(transvalues1[1]) || isinf(transvalues2[1]), true);
+ sumX2 += transvalues1[2];

The last line should use transvalues2[2], not transvalues1[2].

There's also quite a few mistakes in float8_regr_combine()

+ sumX2 += transvalues2[2];
+ CHECKFLOATVAL(sumX2, isinf(transvalues1[2]) || isinf(transvalues2[1]), true);

Wrong array element on isinf() check

+ sumY2 += transvalues2[4];
+ CHECKFLOATVAL(sumY2, isinf(transvalues1[4]) || isinf(transvalues2[3]), true);

Wrong array element on isinf() check

+ sumXY += transvalues2[5];
+ CHECKFLOATVAL(sumXY, isinf(transvalues1[5]) || isinf(transvalues2[1]) ||
+ isinf(transvalues2[3]), true);

Wrong array element on isinf() check, and the final
isinf(transvalues2[3]) check does not need to be there.

I've re-based the patch and fixed these already, so will send updated
patches shortly.

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