From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Rowley <david.rowley@2ndquadrant.com>
cc: Stephen Frost <sfrost@snowman.net>,
        Tomas Vondra <tomas.vondra@2ndquadrant.com>,
        Andres Freund <andres@anarazel.de>,
        PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Parallel Aggregates for string_agg and array_agg
In-reply-to: <6538.1522096067@sss.pgh.pa.us>
References: 
 <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>
 <20180326022647.GL24540@tamriel.snowman.net>
 <CAKJS1f-axkksX3XzduLJuTL=o8CSwf1NaQYO6=iHSXR_B3pCKQ@mail.gmail.com>
 <20180326132009.GN24540@tamriel.snowman.net>
 <CAKJS1f8JVpUUkYP2c+RyVRdpS1dnjB_DA5fbcDSL3qfMRqc81Q@mail.gmail.com>
 <6538.1522096067@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Mon, 26 Mar 2018 16:27:47 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <25811.1522108164.1@sss.pgh.pa.us>
Date: Mon, 26 Mar 2018 19:49:24 -0400
Message-ID: <25812.1522108164@sss.pgh.pa.us>
Precedence: bulk

I wrote:
> The main thing that remains undone is to get some test coverage ---
> AFAICS, none of these new functions get exercised in the standard
> regression tests.

Oh, I thought of another thing that would need to get done, if we decide
to commit this.  array_agg_serialize/deserialize only work if the array
element type has send/receive functions.  The planner's logic to decide
whether partial aggregation is possible doesn't look any deeper than
whether the aggregate has serialize/deserialize, but I think we have to
teach it that if it's these particular serialize/deserialize functions,
it needs to check the array element type.  Otherwise we'll get runtime
failures if partial aggregation is attempted on array_agg().

This would not matter for any core datatypes, since they all have
send/receive functions, but I imagine it's still pretty common for
extension types not to.

For my money it'd be sufficient to hard-code the test like

    if ((aggserialfn == F_ARRAY_AGG_SERIALIZE ||
         aggdeserialfn == F_ARRAY_AGG_DESERIALIZE) &&
        !array_element_type_has_send_and_recv(exprType(aggregate input)))

Someday we might need more flexible testing, but that can be solved
some other time.

			regards, tom lane