From: Stephen Frost <sfrost@snowman.net>
To: David Rowley <david.rowley@2ndquadrant.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Parallel Aggregates for string_agg and array_agg
Date: Sun, 25 Mar 2018 22:26:47 -0400
Message-ID: <20180326022647.GL24540@tamriel.snowman.net> (raw)
In-Reply-To: <CAKJS1f-bKAD+s-CJetRQPjmrR-gF-kfvwfzhQm-A0PA-nsr1xw@mail.gmail.com>
References: <20180301212631.kede4nyuttzxwoxq@alap3.anarazel.de>
<CAKJS1f-T=Ryw9OVG6hUtJ4hJoBUtM3q3bZkbCko6nU9pv+vi4A@mail.gmail.com>
<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>
Greetings,
* David Rowley (david.rowley@2ndquadrant.com) wrote:
> On 16 March 2018 at 13:46, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
> > 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.
>
> 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/adt/array_userfuncs.c
[...]
+ for (i = 0; i < state2->nelems; i++)+ state1->dvalues[i] = 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 = 0; i < state->nelems; i++)
+ {
+ outputbytes = 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 = 0; i < nelems; i++)
+ {
+ StringInfoData tmp;
+ tmp.cursor = 0;
+ tmp.maxlen = tmp.len = pq_getmsgint(&buf, 4);
+ tmp.data = (char *) pq_getmsgbytes(&buf, tmp.len);
+
+ result->dvalues[i] = OidReceiveFunctionCall(iodata->typreceive,
+ &tmp,
+ iodata->typioparam,
+ -1);
+ }
+ }
+
+ /* dnulls */
+ temp = 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
Attachments:
signature.asc (application/pgp-signature, 818B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sfrost@snowman.net, david.rowley@2ndquadrant.com, tomas.vondra@2ndquadrant.com, andres@anarazel.de
Subject: Re: Parallel Aggregates for string_agg and array_agg
In-Reply-To: <20180326022647.GL24540@tamriel.snowman.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox