Subject: Re: Parallel Aggregates for string_agg and array_agg
To: David Rowley <david.rowley@2ndquadrant.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Stephen Frost <sfrost@snowman.net>,
 Andres Freund <andres@anarazel.de>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: 
 <CAKJS1f8cB3cc5AK=wPvByhU_jKkgd81iRt2kE6cru8uxx2HCuA@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> <25812.1522108164@sss.pgh.pa.us>
 <CAKJS1f87rFqL0=ksfs+3mmQvg_R0RRGEXyayxHkwYnFqeKpngQ@mail.gmail.com>
 <CAKJS1f9oTWG4oc7B2WfDhT11KhxeV1miLQ+PzB6hY=iJeHM28A@mail.gmail.com>
 <19216.1522207699@sss.pgh.pa.us> <18741.1522245278@sss.pgh.pa.us>
 <1ffa699d-ff84-4c21-10cb-4d8b36017bab@2ndquadrant.com>
 <CAKJS1f95uF3h0PdMj-zqnTcJXnF68bT4nUKgiXtAdEz44W5HWg@mail.gmail.com>
 <13e3fd13-8152-72be-4e3c-8095e6ec0995@2ndquadrant.com>
 <CAKJS1f-S493AXpKAiri7cSFfhy2vy5kLHzkTDXC5QzoxMJ5Lkg@mail.gmail.com>
 <73ea70fa-9ada-deb5-549e-02fa2f0969c6@2ndquadrant.com>
 <CAKJS1f9jP_b=yoLa5bt9WPC5mtNgMKFgP6FH2ejZ_PqYyxbDAQ@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <ad2283b9-64bd-3016-da60-648ba9c13c5f@2ndquadrant.com>
Date: Wed, 4 Apr 2018 21:12:54 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: 
 <CAKJS1f9jP_b=yoLa5bt9WPC5mtNgMKFgP6FH2ejZ_PqYyxbDAQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 03/31/2018 04:42 PM, David Rowley wrote:
> On 30 March 2018 at 02:55, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>> On 03/29/2018 03:09 PM, David Rowley wrote:
>>> I meant to mention earlier that I coded
>>> agg_args_have_sendreceive_funcs() to only check for send/receive
>>> functions. Really we could allow a byval types without send/receive
>>> functions, since the serial/deserial just send the raw datums in that
>>> case, but then the function becomes
>>> agg_byref_args_have_sendreceive_funcs(), which seemed a bit obscure,
>>> so I didn't do that.  Maybe I should?
>>
>> I'd do that. Not sure the function name needs to change, but perhaps
>> agg_args_support_sendreceive() would be better - it covers both byref
>> types (which require send/receive functions) and byval (which don't).
> 
> The attached patch implements this.
> 

Seems fine to me, although we should handle the anyarray case too, I
guess. That is, get_agg_clause_costs_walker() should do this too:

    /* Same thing for array_agg_array_(de)serialize. */
    if ((aggserialfn == F_ARRAY_AGG_ARRAY_SERIALIZE ||
         aggdeserialfn == F_ARRAY_AGG_ARRAY_DESERIALIZE) &&
         !agg_args_support_sendreceive(aggref))
        costs->hasNonSerial = true;

Other than that, the patch seems fine to me, and it's already marked as
RFC so I'll leave it at that.

The last obstacle seems to be the argument about the risks of the patch
breaking queries of people relying on the ordering. Not sure what's are
the right next steps in this regard ...

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services