On 01/29/2015 12:28 AM, Peter Geoghegan wrote:
> On Mon, Jan 26, 2015 at 11:21 PM, Andreas Karlsson <andreas@proxel.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?
Because there is no declaration of numeric_stddev_internal() either. If
there should be I could add declarations for both.
> * I concur with others - we should stick to int16 for the SQL
> interface. The inconsistency there is perhaps slightly confusing, but
> that's historic.
Agreed.
> * 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.
Not entirely sure exactly what I mean but I have changed to something
like this, relying on typedef, in the attached version of the patch. I
think it looks better after the changes.
> * 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.
Fixed.
I have attached a new version of the patch which fixes the issues above
plus moves the autoconf code to the right place (from configure.in to
c-compiler.m4).
--
Andreas Karlsson