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

Hi,

> +# Check if platform support gcc style 128-bit integers.
> +AC_CHECK_TYPES([__int128_t, __uint128_t], [], [], [])

Hm, I'm not sure that's sufficent. Three things:

a) Afaics only __int128/unsigned __int128 is defined. See  https://gcc.gnu.org/onlinedocs/gcc/_005f_005fint128.html

b) I'm doubtful that AC_CHECK_TYPES is a sufficiently good test on all  platforms. IIRC gcc will generate calls to
functionsto 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
arithmeticstill links.
 

c) Personally I don't see the point of testing __uint128_t. That's just  a pointless test that makes configure run for
longer.

> +#ifdef HAVE_INT128
> +typedef struct Int16AggState
> +{
> +    bool    calcSumX2;    /* if true, calculate sumX2 */
> +    int64    N;            /* count of processed numbers */
> +    int128    sumX;        /* sum of processed numbers */
> +    int128    sumX2;        /* sum of squares of processed numbers */
> +} Int16AggState;

Not the biggest fan of the variable naming here, but that's not your
fault, you're just consistent which is good.


> @@ -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.

> --- a/src/include/pg_config.h.in
> +++ b/src/include/pg_config.h.in
> @@ -678,6 +678,12 @@
>  /* Define to 1 if your compiler understands __VA_ARGS__ in macros. */
>  #undef HAVE__VA_ARGS
>  
z> +/* Define to 1 if the system has the type `__int128_t'. */
> +#undef HAVE___INT128_T
> +
> +/* Define to 1 if the system has the type `__uint128_t'. */
> +#undef HAVE___UINT128_T

pg_config.h.win32 should be updated as well.

Greetings,

Andres Freund