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

On 2015-01-11 05:07:13 +0100, Andreas Karlsson wrote:
> On 01/11/2015 02:36 AM, Andres Freund wrote:
> >a) Afaics only __int128/unsigned __int128 is defined. See
> >    https://gcc.gnu.org/onlinedocs/gcc/_005f_005fint128.html
> 
> Both GCC and Clang defines both of them. Which you use seems to just be a
> matter of preference.

Only __int128 is documented, the other stuff is just legacy (and
internally documented to be just backward compat stuff).

> >b) I'm doubtful that AC_CHECK_TYPES is a sufficiently good test on all
> >    platforms. IIRC gcc will generate calls to functions to do the actual
> >    arithmetic, and I don't think they're guranteed to be available on
> >    platforms. That how it .e.g. behaves for atomic operations. So my
> >    guess is that you'll need to check that a program that does actual
> >    arithmetic still links.
> 
> I too thought some about this and decided to look at how other projects
> handled this. The projects I have looked at seems to trust that if
> __int128_t is defined it will also work.
> 
> https://github.com/python/cpython/blob/master/configure#L7778
> http://cgit.freedesktop.org/cairo/tree/build/configure.ac.system#n88
> 
> But after some more searching now I notice that at least gstreamer does not
> trust this to be true.
> 
> https://github.com/ensonic/gstreamer/blob/master/configure.ac#L382
> 
> Should I fix it to actually compile some code which uses the 128-bit types?

Yes, compile + link please. As Tom said, best also test some arithmetics.

> >>@@ -3030,6 +3139,18 @@ int8_avg_accum(PG_FUNCTION_ARGS)
> >>  Datum
> >>  int2_accum_inv(PG_FUNCTION_ARGS)
> >>  {
> >>+#ifdef HAVE_INT128
> >>+    Int16AggState *state;
> >>+
> >>+    state = PG_ARGISNULL(0) ? NULL : (Int16AggState *) PG_GETARG_POINTER(0);
> >>+
> >>+    /* Should not get here with no state */
> >>+    if (state == NULL)
> >>+        elog(ERROR, "int2_accum_inv called with NULL state");
> >>+
> >>+    if (!PG_ARGISNULL(1))
> >>+        do_int16_discard(state, (int128) PG_GETARG_INT16(1));
> >>+#else
> >>      NumericAggState *state;
> >>
> >>      state = PG_ARGISNULL(0) ? NULL : (NumericAggState *) PG_GETARG_POINTER(0);
> >>@@ -3049,6 +3170,7 @@ int2_accum_inv(PG_FUNCTION_ARGS)
> >>          if (!do_numeric_discard(state, newval))
> >>              elog(ERROR, "do_numeric_discard failed unexpectedly");
> >>      }
> >
> >Hm. It might be nicer to move the if (!state) elog() outside the ifdef,
> >and add curly parens inside the ifdef.
> 
> The reason I did so was because the type of the state differs and I did not
> feel like having two ifdef blocks. I have no strong opinion about this
> though.

Petr explained what I was thinking of.

> >pg_config.h.win32 should be updated as well.
> 
> Is it possible to update it without running Windows?

Just copy the relevant hunks by hand. In your case the #undef's
generated are sufficient. There are others where we want to define
things unconditionally.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services