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

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
Date: 2015-03-04 22:51:42
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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 to

Andreas Karlsson

Attachment Content-Type Size
int128-agg-v6.patch text/x-patch 36.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
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"