|From:||Andreas Karlsson <andreas(at)proxel(dot)se>|
|To:||Peter Geoghegan <pg(at)heroku(dot)com>|
|Cc:||Petr Jelinek <petr(at)2ndquadrant(dot)com>, David Rowley <dgrowleyml(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)2ndquadrant(dot)com>|
|Subject:||Re: Using 128-bit integers for sum, avg and statistics aggregates|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 01/29/2015 12:28 AM, Peter Geoghegan wrote:
> 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?
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.
> * 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.
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
|Next Message||David Kubečka||2015-03-04 23:08:10||Re: Weirdly pesimistic estimates in optimizer|
|Previous Message||Andres Freund||2015-03-04 22:08:09||Re: Idea: closing the loop for "pg_ctl reload"|