Subject: Re: Parallel Aggregates for string_agg and array_agg
To: David Rowley <david.rowley@2ndquadrant.com>,
 Andres Freund <andres@anarazel.de>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
References: 
 <CAKJS1f9sx_6GTcvd6TMuZnNtCh0VhBzhX6FZqw17TgVFH-ga_A@mail.gmail.com>
 <20180301212631.kede4nyuttzxwoxq@alap3.anarazel.de>
 <CAKJS1f-T=Ryw9OVG6hUtJ4hJoBUtM3q3bZkbCko6nU9pv+vi4A@mail.gmail.com>
 <CAKJS1f8cB3cc5AK=wPvByhU_jKkgd81iRt2kE6cru8uxx2HCuA@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <c302afa6-6415-27a5-336a-e1d4332fab09@2ndquadrant.com>
Date: Sun, 4 Mar 2018 16:54:07 +0100
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: 
 <CAKJS1f8cB3cc5AK=wPvByhU_jKkgd81iRt2kE6cru8uxx2HCuA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

I've looked at the rebased patch you sent yesterday, and I do have a
couple of comments.

1) There seems to be forgotten declaration of initArrayResultInternal in
arrayfuncs.c. I suppose you've renamed it to initArrayResultWithSize and
moved it to a header file, and forgotten to remove this bit.

2) bytea_string_agg_deserialize has this comment:

 /*
  * data: technically we could reuse the buf.data buffer, but that is
  * slightly larger than we need due to the extra 4 bytes for the cursor
  */

I find the argument "it has 4 extra bytes, so we'll allocate new chunk"
somewhat weak. We do allocate the memory in 2^n chunks anyway, so the
new chunk is likely to be much larger anyway. I'm not saying we need to
reuse the buffer, IMHO the savings will be non-measurable.

3) string_agg_transfn and bytea_string_agg_transfn say this"

 /*
  * We always append the delimiter text before the value text, even
  * with the first aggregated item.  The reason for this is that if
  * this state needs to be combined with another state using the
  * aggregate's combine function, then we need to have the delimiter
  * for the first aggregated item.  The first delimiter will be
  * stripped off in the final function anyway.  We use a little cheat
  * here and store the position of the actual first value (after the
  * delimiter) using the StringInfo's cursor variable.  This relies on
  * the cursor not being used for any other purpose.
  */

How does this make the code any simpler, compared to not adding the
delimiter (as before) and adding it when combining the values (at which
point you should know when it's actually needed)?

regards

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