On 22 June 2018 at 02:01, Tom Lane <t...@sss.pgh.pa.us> wrote: > David Rowley <david.row...@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
fix_numeric_poly_combine_v2.patch
Description: Binary data
