Greetings, * David Rowley (david.row...@2ndquadrant.com) wrote: > On 26 March 2018 at 15:26, Stephen Frost <sfr...@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
signature.asc
Description: PGP signature
