Date: Mon, 26 Mar 2018 09:20:09 -0400
From: Stephen Frost <sfrost@snowman.net>
To: David Rowley <david.rowley@2ndquadrant.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	Andres Freund <andres@anarazel.de>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Parallel Aggregates for string_agg and array_agg
Message-ID: <20180326132009.GN24540@tamriel.snowman.net>
References: 
 <CAKJS1f8cB3cc5AK=wPvByhU_jKkgd81iRt2kE6cru8uxx2HCuA@mail.gmail.com>
 <c302afa6-6415-27a5-336a-e1d4332fab09@2ndquadrant.com>
 <CAKJS1f9xEZdTaBbBEKY+TCKEoSZYaH1oSAtsEJHSYDJCCNnQPQ@mail.gmail.com>
 <24abe018-b3c6-2117-b5b9-39e660db00bd@2ndquadrant.com>
 <CAKJS1f8LV7AT=AAhdYGKtGrGkSkEgO6C_SW2Ztz1sR3encisqw@mail.gmail.com>
 <1b6ee15c-c218-761e-0120-c1ed438e2db2@2ndquadrant.com>
 <657be73a-a21d-0ae1-5c62-870afbdfddd4@2ndquadrant.com>
 <CAKJS1f-bKAD+s-CJetRQPjmrR-gF-kfvwfzhQm-A0PA-nsr1xw@mail.gmail.com>
 <20180326022647.GL24540@tamriel.snowman.net>
 <CAKJS1f-axkksX3XzduLJuTL=o8CSwf1NaQYO6=iHSXR_B3pCKQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="YC8Ek3FeOE8ywfXk"
Content-Disposition: inline
In-Reply-To: 
 <CAKJS1f-axkksX3XzduLJuTL=o8CSwf1NaQYO6=iHSXR_B3pCKQ@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--YC8Ek3FeOE8ywfXk
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> Thanks for looking at this.
>=20
> 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

--YC8Ek3FeOE8ywfXk
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJauPOJAAoJEO1sijiDR2RVS54P/2bm6EZ2jTQGW8RkGoMcjdfn
eY3fEDrv/7+BJ6gFLXkGdYSz944lDE8WJd1stztbo9BvN8D2RinkwtrMXc1pa9Gj
BoLKNRPocjVeKmzYtJKnnGlpNroUjasAcrOFsNujM5SZ/l1d0MrK21gjI6va+G/Q
DCdgs8efL8Go46zzUGmAnt/cyOyNvMOkWaWzs55DEG145vsDqTJDi9auwa1/k4pF
KX+LRyUp9aH4ok5aYHZnfQX5yemeQGc90rnTXoRhFI8Q8/CGLK6efzIv9ZoT86mS
+v8g+g86sZUwgrDtUD39rwuIJhre5UcwQ/IYG27KQD2TGv69OgnCVT7USIt2/k1G
patgwGofHSbJGc/GgfHJlEGVHOJU5UI6pOTUVC10PCxQ34vlUC1CMsTNSyBoxT/Z
cbgPdtUeCseH83hkg+ApCLkD1ANryUpVqyETdWtJBrm7tRSgx1OeJB3u7K0KaaE4
T8nhxfJq+FaLMK7zMH9SzP8JNQh8j939fzwnI90ZcG0BDO8u0JhLf39Qqw3jNXZm
Qa9nDRlKFcpCcxbfG6gIdqqwdaDKNMEwTo4MW+aJODBp07VhD3QKdEx7y/rL4NXn
uqxoaS51xrV/9EedrBDPq2oRWjgqG1FYM/GgW9BIB7U9cBk/dZLByzOD1XtMPW/2
Z5SkML6RIRtlPa/jerDE
=UwfY
-----END PGP SIGNATURE-----

--YC8Ek3FeOE8ywfXk--