Re: Review: listagg aggregate

2010/1/28 Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>:
>
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
>> with actualised oids
>
> I'm checking the patch for commit, and have a couple of comments.
>
> * 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'.

no I dislike it. This using is nonsense.

Regards
Pavel

>
> * 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.
>
> * 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.
>
> Comments?
>
> Regards,
> ---
> Takahiro Itagaki
> NTT Open Source Software Center
>
>