From: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
---|---|
To: | Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> |
Cc: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, David Steele <david(at)pgmasters(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Amit Kapila <amit(dot)kapila(at)enterprisedb(dot)com> |
Subject: | Re: Combining Aggregates |
Date: | 2016-03-20 03:23:03 |
Message-ID: | CAKJS1f-BoOM8crDSn1K8dbzrMiAE0b2yAg15ze0EuZ-DLMnHfg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | David Rowley | 2016-03-20 03:48:44 | Re: Combining Aggregates |
Previous Message | Andres Freund | 2016-03-20 01:45:36 | Re: Performance degradation in commit ac1d794 |