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>
 <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>
 <ad2283b9-64bd-3016-da60-648ba9c13c5f@2ndquadrant.com>
 <CAKJS1f_yDsOrSffHBqAN3Gx27R1vW6OHaKy_EwnhnLZJmo8+Mw@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <8f76820a-5d3a-bade-59c3-a3852b730826@2ndquadrant.com>
Date: Thu, 5 Apr 2018 22:49:03 +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: 
 <CAKJS1f_yDsOrSffHBqAN3Gx27R1vW6OHaKy_EwnhnLZJmo8+Mw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 04/05/2018 05:41 AM, David Rowley wrote:
> Hi Tomas,
> 
> Thanks for taking another look.
> 
> On 5 April 2018 at 07:12, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>> 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;
> 
> hmm, array_agg_array_serialize and array_agg_array_deserialize don't
> use the send/receive functions though, so not sure why that's
> required?
> 
>> Other than that, the patch seems fine to me, and it's already marked as
>> RFC so I'll leave it at that.
> 
> Thanks.
> 
>> 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 ...
> 
> yeah, seems like a bit of a stalemate.
> 
> Personally, think if the group of people Tom mentions do exist, then
> they've already been through some troubled times since Parallel Query
> was born. I'd hope they've already put up their defenses due to the
> advent of that feature. I stand by my thoughts that it's pretty
> bizarre to draw the line here when we've probably been causing these
> people issues for many years already. I said my piece on this already
> so likely not much point in going on about it. These people are also
> perfectly capable of sidestepping the whole issue with setting
> max_parallel_workers_per_gather TO 0.
> 
> Perhaps one solution is to drop the string_agg and keep the array_agg.
> Unsure if that would be good enough for Tom? More people seem to have
> voiced that array_agg ordering is generally less of a concern, which I
> imagine is probably true, but my opinion might not matter here as I'm
> the sort of person who if I needed a specific ordering I'd have
> written an ORDER BY clause.
> 

I don't really see how dropping string_agg would solve anything. Either
the risks apply to both these functions, or neither of them I think. If
anything, doing this only for one of them would be inconsistent and
surprising, considering how similar they are.

regards

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