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