Re: Using 128-bit integers for sum, avg and statistics aggregates

On Mon, Jan 26, 2015 at 11:21 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> Do you also think the SQL functions should be named numeric_int128_sum,
> numeric_int128_avg, etc?

Some quick review comments. These apply to int128-agg-v5.patch.

* Why is there no declaration of the function
numeric_int16_stddev_internal() within numeric.c?

* I concur with others - we should stick to int16 for the SQL
interface. The inconsistency there is perhaps slightly confusing, but
that's historic.

* I'm not sure about the idea of "polymorphic" catalog functions (that
return the type "internal", but the actual struct returned varying
based on build settings).

I tend to think that things would be better if there was always a
uniform return type for such "internal" type returning functions, but
that *its* structure varied according to the availability of int128
(i.e. HAVE_INT128) at compile time. What we should probably do is have
a third aggregate struct, that encapsulates this idea of (say)
int2_accum() piggybacking on one of either Int128AggState or
NumericAggState directly. Maybe that would be called PolyNumAggState.
Then this common code is all that is needed on both types of build (at
the top of int4_accum(), for example):

PolyNumAggState *state;

state = PG_ARGISNULL(0) ? NULL : (PolyNumAggState *) PG_GETARG_POINTER(0);

I'm not sure if I'd do this with a containing struct or a simple
"#ifdef HAVE_INT128 typedef #else ... ", but I think it's an
improvement either way.

* You didn't update this unequivocal comment to not be so strong:

* Integer data types all use Numeric accumulators to share code and
* avoid risk of overflow.

That's all I have for now...
--
Peter Geoghegan