On 18 March 2016 at 13:39, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> On Thu, Mar 17, 2016 at 10:59 PM, Tomas Vondra
> <tomas.vondra@2ndquadrant.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
