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

Hi,

On 2017-12-18 03:30:55 +1300, David Rowley wrote:
> While working on partial aggregation a few years ago, I didn't really
> think it was worthwhile allowing partial aggregation of string_agg and
> array_agg. I soon realised that I was wrong about that and allowing
> parallelisation of these aggregates still could be very useful when
> many rows are filtered out during the scan.

+1


> Some benchmarks that I've done locally show that parallel string_agg
> and array_agg do actually perform better, despite the fact that the
> aggregate state grows linearly with each aggregated item.

It also allows *other* aggregates with different space consumption to be
computed in parallel, that can be fairly huge.


> 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'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?


> +/*
> + * array_agg_serialize
> + *        Serialize ArrayBuildState into bytea.
> + */
> +Datum
> +array_agg_serialize(PG_FUNCTION_ARGS)
> +{

> +    /*
> +     * dvalues -- this is very simple when the value type is byval, we can
> +     * simply just send the Datums over, however, for non-byval types we must
> +     * work a little harder.
> +     */
> +    if (state->typbyval)
> +        pq_sendbytes(&buf, (char *) state->dvalues, sizeof(Datum) * state->nelems);
> +    else
> +    {
> +        Oid            typsend;
> +        bool        typisvarlena;
> +        bytea       *outputbytes;
> +        int            i;
> +
> +        /* 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?

> +
> +Datum
> +array_agg_deserialize(PG_FUNCTION_ARGS)
> +{

> +    /*
> +     * dvalues -- this is very simple when the value type is byval, we can
> +     * simply just get all the Datums at once, however, for non-byval types we
> +     * must work a little harder.
> +     */
> +    if (result->typbyval)
> +    {
> +        temp = pq_getmsgbytes(&buf, sizeof(Datum) * nelems);
> +        memcpy(result->dvalues, temp, sizeof(Datum) * nelems);
> +    }
> +    else
> +    {
> +        Oid        typreceive;
> +        Oid        typioparam;
> +        int        i;
> +
> +        getTypeBinaryInputInfo(element_type, &typreceive, &typioparam);
> +
> +        for (i = 0; i < nelems; i++)
> +        {
> +            /* 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?


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

Greetings,

Andres Freund