Re: Review: listagg aggregate

2010/1/28 David E. Wheeler <david@kineticode.com>:
> On Jan 27, 2010, at 6:47 PM, Takahiro Itagaki wrote:
>
>> * I think we cannot cache the delimiter at the first call.
>>  For example,
>>    SELECT string_agg(elem, delim)
>>      FROM (VALUES('A', ','), ('B', '+'), ('C', '*')) t(elem, delim);
>>  should return 'A+B*C' rather than 'A,B,C'.
>
> Ooh, nice.
>
>> * Can we use StringInfo directly as the aggregate context instead of
>>  StringAggState? For the first reason, we need to drop 'delimiter' field
>>  from struct StringAggState. Now it has only StringInfo field.
>
> Makes sense.

no, has not.

Pavel

>
>> * We'd better avoiding to call text_to_cstring() for delimitors and elements
>>  for performance reason. We can use appendBinaryStringInfo() here.
>>
>> My proposal patch attached.
>>
>> Also, I've not changed it yet, but it might be considerable:
>>
>> * Do we need better names for string_agg1_transfn and string_agg2_transfn?
>>  They are almost "internal names", but we could have more
>>  like string_agg_with_sep_transfn.
>
> Yes please.
>
>> Comments?
>
> Patch looks great, thank you!
>
> David
>
>
>