Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f0HqX-0006np-P6 for pgsql-hackers@arkaria.postgresql.org; Mon, 26 Mar 2018 02:26:53 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1f0HqV-0002Ax-OL for pgsql-hackers@arkaria.postgresql.org; Mon, 26 Mar 2018 02:26:51 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f0HqV-0002An-Dm for pgsql-hackers@lists.postgresql.org; Mon, 26 Mar 2018 02:26:51 +0000 Received: from tamriel.snowman.net ([96.255.250.162]) by makus.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1f0HqS-0000xo-G1 for pgsql-hackers@postgresql.org; Mon, 26 Mar 2018 02:26:49 +0000 Received: by tamriel.snowman.net (Postfix, from userid 1000) id A5A415F79C; Sun, 25 Mar 2018 22:26:47 -0400 (EDT) Date: Sun, 25 Mar 2018 22:26:47 -0400 From: Stephen Frost To: David Rowley Cc: Tomas Vondra , Andres Freund , PostgreSQL-development Subject: Re: Parallel Aggregates for string_agg and array_agg Message-ID: <20180326022647.GL24540@tamriel.snowman.net> References: <20180301212631.kede4nyuttzxwoxq@alap3.anarazel.de> <24abe018-b3c6-2117-b5b9-39e660db00bd@2ndquadrant.com> <1b6ee15c-c218-761e-0120-c1ed438e2db2@2ndquadrant.com> <657be73a-a21d-0ae1-5c62-870afbdfddd4@2ndquadrant.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="4ybNbZnZ8tziJ7D6" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --4ybNbZnZ8tziJ7D6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Greetings, * David Rowley (david.rowley@2ndquadrant.com) wrote: > On 16 March 2018 at 13:46, Tomas Vondra wr= ote: > > I've done more testing on this patch, and I haven't found any new issues > > with it, so PFA I'm marking it as ready for committer. >=20 > Great! Many thanks for the review. I read through the thread and started reviewing this, comments below. diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/ad= t/array_userfuncs.c [...] + for (i =3D 0; i < state2->nelems; i++) + state1->dvalues[i] =3D datumCopy(state2->dvalues[i], + state1->typbyval, state1->typlen); + + memcpy(state1->dnulls, state2->dnulls, sizeof(bool) * state2->nelems); 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. [...] + for (i =3D 0; i < state->nelems; i++) + { + outputbytes =3D OidSendFunctionCall(iodata->typsend, state->dvalues[i]); + pq_sendint32(&buf, VARSIZE(outputbytes) - VARHDRSZ); + pq_sendbytes(&buf, VARDATA(outputbytes), + VARSIZE(outputbytes) - VARHDRSZ); + } + } + + /* dnulls */ + pq_sendbytes(&buf, (char *) state->dnulls, sizeof(bool) * state->nelems); SendFunctionCall() has a similar "Do not call this on NULL datums" comment. + for (i =3D 0; i < nelems; i++) + { + StringInfoData tmp; + tmp.cursor =3D 0; + tmp.maxlen =3D tmp.len =3D pq_getmsgint(&buf, 4); + tmp.data =3D (char *) pq_getmsgbytes(&buf, tmp.len); + + result->dvalues[i] =3D OidReceiveFunctionCall(iodata->typreceive, + &tmp, + iodata->typioparam, + -1); + } + } + + /* dnulls */ + temp =3D pq_getmsgbytes(&buf, sizeof(bool) * nelems); + memcpy(result->dnulls, temp, sizeof(bool) * nelems); And if we aren't sending them then we probably need to do something different on the receive side... I glanced through the rest and it seemed alright, but will want to still go over it more carefully once the above comments are addressed. Marking Waiting-for-Author. Thanks! Stephen --4ybNbZnZ8tziJ7D6 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJauFpnAAoJEO1sijiDR2RVaBIQAJlbVMwPhyRCCDTuiaBDEGSH igmo1XNlyNW+shX2eJjZZZKGXK19M0AbfrYd3iA1/xqudx3oC5GKrbsBxFesgiwc ctTYL8OldnLnb23nkuUTLWxKQXLxwB8T25v6327dQGCI2r321NJOzJrnj5yk0WC8 h22Ah19a45uklPs65K1+ha9UrdNamUs0JwIVyQG66WzpE6hChB0h7dkJpwpomrkU I0WOPjNQWTktB/6bmO33kvfgGYrU7vGEinyIH3eLn9q4OvJdCbYMJmHCqt3ZoYCC xRvyew2micS/nmuBw6ujifEqPE9zSRrKHayMsnvrYEPw0F/b0b0x8u6I2o3shFmJ y2m56m/i4k/BclCRGkMri69Vx+L6+tUWlimO5hPyOsUKJUrYLt8j1sPsoOywQls+ kLq0BZ2hc43ppIbfYaswdcmjuWKBSvzSZZJIlkvD/IPib4/Gj7Nksyxkw7b2MYDe nf0zt2vPhonTrMkhFcfS2+hCZS5SVyxuBZOLvYrXn2pr+ALMZaZPqd6Jho+Ib7qK Lp0mcEiYyZuVP94n9EXD/945+4yY2oGF+QQ0ngd3c8WJ0KSJJaD6Gmp4pCctV7m9 1rUBM6LF+wPMx7y0rXqSUrdSLveSxjA/7PrCdCgc9e/po/I8oBoY+6M8i1+cCNsx 1n4O/IoXF1GgTiWUkW8k =O7b/ -----END PGP SIGNATURE----- --4ybNbZnZ8tziJ7D6--