From: | Andreas Karlsson <andreas(at)proxel(dot)se> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | David Rowley <dgrowleyml(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Using 128-bit integers for sum, avg and statistics aggregates |
Date: | 2015-01-11 04:07:13 |
Message-ID: | 54B1F6F1.7080805@proxel.se |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
> 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?
> c) Personally I don't see the point of testing __uint128_t. That's just
> a pointless test that makes configure run for longer.
Ok, will remove it in the next version of the patch.
>> @@ -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.
> pg_config.h.win32 should be updated as well.
Is it possible to update it without running Windows?
--
Andreas Karlsson
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2015-01-11 04:14:47 | Re: Parallel Seq Scan |
Previous Message | Robert Haas | 2015-01-11 03:46:07 | Re: Parallel Seq Scan |