Re: Parallel Aggregates for string_agg and array_agg - Mailing list pgsql-hackers

From David Rowley
Subject Re: Parallel Aggregates for string_agg and array_agg
Date
Msg-id CAKJS1f-T=Ryw9OVG6hUtJ4hJoBUtM3q3bZkbCko6nU9pv+vi4A@mail.gmail.com
Whole thread Raw
In response to Re: Parallel Aggregates for string_agg and array_agg  (Andres Freund <andres@anarazel.de>)
Responses Re: Parallel Aggregates for string_agg and array_agg  (Andres Freund <andres@anarazel.de>)
Re: Parallel Aggregates for string_agg and array_agg  (David Rowley <david.rowley@2ndquadrant.com>)
List pgsql-hackers
On 2 March 2018 at 10:26, Andres Freund <andres@anarazel.de> wrote:
> On 2017-12-18 03:30:55 +1300, David Rowley wrote:
>> Just a handful of aggregates now don't support partial aggregation;
>>
>> postgres=# select aggfnoid from pg_aggregate where aggcombinefn=0 and
>> aggkind='n';
>>      aggfnoid
>> ------------------
>>  xmlagg
>>  json_agg
>>  json_object_agg
>>  jsonb_agg
>>  jsonb_object_agg
>> (5 rows)
>
> FWIW, I've heard numerous people yearn for json*agg

I guess it'll need to be PG12 for now. I'd imagine string_agg and
array_agg are more important ones to tick off for now, but I bet many
people would disagree with that.

>
>> I'm going to add this to PG11's final commitfest rather than the
>> January 'fest as it seems more like a final commitfest type of patch.
>
> Not sure I understand?

hmm, yeah. That was more of a consideration that I should be working
hard on + reviewing more complex and invasive patches before the final
'fest. I saw this more as something that would gain more interest once
patches such as partition-wise aggs gets in.

>> +             /* XXX is there anywhere to cache this to save calling each time? */
>> +             getTypeBinaryOutputInfo(state->element_type, &typsend, &typisvarlena);
>
> Yes, you should be able to store this in fcinfo->flinfo->fn_extra. Or do
> you see a problem doing so?

No, just didn't think of it.

>> +                     /* XXX? Surely this cannot be the way to do this? */
>> +                     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(typreceive, &tmp,
>> +
typioparam,-1);
 
>
> Hm, doesn't look too bad to me? What's your concern here?

I was just surprised at having to fake up a StringInfoData.

> This isn't a real review, I was basically just doing a quick
> scroll-through.

Understood.

I've attached a delta patch which can be applied on top of the v2
patch which removes that XXX comment above and also implements the
fn_extra caching.

Thanks for looking!

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Attachment

pgsql-hackers by date:

Previous
From: Andres Freund
Date:
Subject: Re: [HACKERS] make async slave to wait for lsn to be replayed
Next
From: Andres Freund
Date:
Subject: Re: Parallel Aggregates for string_agg and array_agg