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

On 11/01/15 05:07, Andreas Karlsson wrote:
> On 01/11/2015 02:36 AM, Andres Freund wrote:
>>> @@ -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.
>

I think Andres means you should move the NULL check before the ifdef and 
then use curly braces inside the the ifdef/else so that you can define 
the state variable there. That can be done with single ifdef.

int2_accum_inv(PG_FUNCTION_ARGS)
{
... null check ...    {
#ifdef HAVE_INT128        Int16AggState *state;
...
#else        NumericAggState *state;
...
#endif        PG_RETURN_POINTER(state);    }
}

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