Re: Parallel Aggregates for string_agg and array_agg

Greetings,

* David Rowley (david.rowley@2ndquadrant.com) wrote:
> On 26 March 2018 at 15:26, Stephen Frost <sfrost@snowman.net> wrote:
> > The header at the top of datumCopy() pretty clearly says that it's for
> > "non-NULL" datums, yet this function seems to be happily ignoring that
> > and just trying to use it to copy everything.  Perhaps I'm missing
> > something, but I don't see anything obvious that would lead me to
> > conclude that what's being done here (or in other similar cases in this
> > patch) is acceptable.
> 
> Thanks for looking at this.
> 
> You're right. I've overlooked this. The code should be checking for
> NULL value Datums there.  I've fixed this locally, but on testing, I
> discovered another bug around string_agg. At the moment string_agg's
> transfn only allocates the state when it gets a non-NULL string to
> aggregate, whereas it seems other trans functions which return an
> internal state allocate their state on the first call. e.g.
> int8_avg_accum().  This NULL state is causing the serial function
> segfault on a null pointer dereference.   I think the fix is to always
> allocate the state in the transfn, but I just wanted to point this out
> before I go and do that.

Just to be clear- the segfault is just happening with your patch and
you're just contemplating having string_agg always allocate state on the
first call, similar to what int8_avg_accum() does?

If so, then, yes, that seems alright to me.

Thanks!

Stephen