On 22 June 2018 at 02:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> Well, that's quite surprising. It appears to be a bug in
>> numeric_poly_combine for machines without a working int128 type. The
>> parameters in accum_sum_copy are in the incorrect order.
>
> Ouch.
Yeah. Looks like this function was correct when it went in. It was
9cca11c915e (v10) that caused the issue.
>> The very minimal fix is attached, but I'll need to go look at where
>> the tests for this have gone.
>
> coverage.postgresql.org shows that numeric_poly_serialize/combine()
> aren't exercised at all by the regression tests. Which is embarrassing
> for this case, but I'm a bit leery of trying to insist on 100% coverage.
>
> It might be a plan to insist on buildfarm coverage for anything with
> platform-varying code in it, in which case there's at least one
> other undertested bit of HAVE_INT128 code in numeric.c.
I'm not familiar with what the coverage tool can do, so maybe there's
an easier way than running a script to check for 0 coverage between
#ifdef / #if and #endif inside all .c files.
This sounds like a longer-term project though, let's get this one fixed first.
I think some coverage of the numerical aggregates is a good idea, so
I've added some in the attached. I managed to get a parallel plan
going with a query to onek, which is pretty cheap to execute. I didn't
touch the bool aggregates. Maybe I should have done that too..?
I also did a quick validation of the other accum_sum_copy usages. All
others look correct.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services