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

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