On Mon, Jan 26, 2015 at 11:21 PM, Andreas Karlsson <andr...@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? * 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
