Re: Parallel Aggregates for string_agg and array_agg

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
Cc: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Parallel Aggregates for string_agg and array_agg
Date: 2018-03-26 02:26:47
Message-ID: 20180326022647.GL24540@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Greetings,

* David Rowley (david(dot)rowley(at)2ndquadrant(dot)com) wrote:
> On 16 March 2018 at 13:46, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro HORIGUCHI 2018-03-26 02:28:41 Re: PQHost() undefined behavior if connecting string contains both host and hostaddr types
Previous Message Stephen Frost 2018-03-26 01:43:25 Re: Add default role 'pg_access_server_files'