Re: Parallel Aggregates for string_agg and array_agg

On 2 March 2018 at 10:26, Andres Freund <andres(at)anarazel(dot)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