Re: aggregate crash

Hi,

On 2020-01-15 19:16:30 -0800, Andres Freund wrote:
> The bug, in a way, exists all the way back, but it's a bit harder to
> create NULL values where the datum component isn't 0.

> To fix I suggest we, in all branches, do the equivalent of adding
> something like:
> diff --git i/src/backend/executor/execExprInterp.c w/src/backend/executor/execExprInterp.c
> index 790380051be..3260a63ac6b 100644
> --- i/src/backend/executor/execExprInterp.c
> +++ w/src/backend/executor/execExprInterp.c
> @@ -4199,6 +4199,12 @@ ExecAggTransReparent(AggState *aggstate, AggStatePerTrans pertrans,
>                                   pertrans->transtypeByVal,
>                                   pertrans->transtypeLen);
>      }
> +    else
> +    {
> +        /* ensure datum component is 0 for NULL transition values */
> +        newValue = (Datum) 0;
> +    }
> +
>      if (!oldValueIsNull)
>      {
>          if (DatumIsReadWriteExpandedObject(oldValue,
> 
> and a comment explaining why it's (now) safe to rely on datum
> comparisons for
>     if (DatumGetPointer(newVal) != DatumGetPointer(pergroup->transValue))

Pushed something along those lines.


> A separate question is whether it's worth adding code to
> e.g. EEO_CASE(EEOP_FUNCEXPR_STRICT) also resetting *op->resvalue to
> (Datum) 0.  I don't personally don't think ensuring the datum is always
> 0 when isnull true is all that helpful, if we can't guarantee it
> everywhere. So I'm a bit loathe to add cycles to places that don't need
> it, and are hot.

I wonder if its worth adding a few valgrind annotations marking values
as undefined when null? Would make it easier to catch such cases in the
future.

Greetings,

Andres Freund