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
