Review: listagg aggregate

Pavel,

My review of your listagg patch.

Submission Review
-----------------
* The diff is a context diff and applies cleanly to HEAD (with just two hunks offset by 2 lines each).

* There is documentation, though I'm not sure it needs to be mentioned in the string functions documentation. No harm in it, I guess.

I would like to see an example, though, and the documentation does not currently explain what each of the parameters are for. In fact, it looks like all the existing aggregates take only one parameter, so there was not previously a need to explain it. But listagg() has an optional second param. I think that the description should explain what it's for.

* There are tests and they look fine.

Usability Review
----------------
* The patch does in fact implement the aggregate function it describes, and OH YES do we want it (I've written my own in SQL a few times).

* No, we don't already have it.

* Yes it follows community-agreed behavior. I'm assuming that there is no special parsing of aggregate functions, so the simple use of commas to separate the two parameters is appropriate, rather than using a keyword like MySQL's SEPARATOR in the group_concat() aggregate.

* No need to have pg_dump support, no dangers that I can see, looks like all the bases have been covered.

Feature Test
------------
* Everything built cleanly, but I got an OID dupe error when I tried to init the DB. Looks like 2997 and 2998 have been used for something else since you created the patch. I changed them to 2995 and 2996 and then it worked.
* The feature appears to work. I didn't see any tests for encodings or other data types, so I ran a few myself and they work fine:

postgres=# select listagg(a, U&'-\0441\043B\043E\043D-') from (values('aaaa'),('bbbb'),('cccc'
listagg
--------------------------
aaaa-слон-bbbb-слон-cccc
(1 row)

postgres=# select listagg(a, U&'\2014') from (values(U&'\0441\043B\043E\043D'),(U&'d\0061t\+000061'),(U&'\0441\043B\043E\043D')) AS g(a);
listagg
----------------
слон—data—слон
(1 row)

postgres=# select listagg(a::text) from (values(1),(2),(3)) AS g(a);
listagg
---------
123
(1 row)

Performance Review
------------------

No performance issues, except that it should be faster than a custom aggregate that does the same thing. To test, I created a quick custom aggregate (no second argument, alas, so listagg() is more flexible) like so:

CREATE OR REPLACE FUNCTION a2s(ANYARRAY)
RETURNS TEXT LANGUAGE SQL AS $$
SELECT array_to_string($1, ',');
$$;

CREATE AGGREGATE string_accum (
SFUNC = array_accum,
BASETYPE = ANYELEMENT,
STYPE = ANYARRAY,
INITCOND = '{}',
FINALFUNC = a2s
);

Then I ran some simple tests (thanks for the clue, depesz):

postgres=# select count(*) from (select string_accum(a) from (values('aaaa'),('bbbb'),('cccc')) AS g(a), generate_series(1,10000) i) AS x(i);
count
-------
1
(1 row)

Time: 1365.382 ms

postgres=# select count(*) from (select listagg(a) from (values('aaaa'),('bbbb'),('cccc')) AS g(a), generate_series(1,10000) i) AS x(i);
count
-------
1
(1 row)

Time: 17.989 ms

So overall, it looks like listagg() is 1-2 orders of magnitude faster. YMMV, and my system is built with --enable-cassert and --enable-debug. Still, good job.

Coding Review
-------------

* Is varchar.c really the best place to put the ListAggState struct and the listagg() function? I grepped the source for array_agg() and it's in src/backend/utils/adt/array_userfuncs.c. Maybe there's an equivalent file for string functions? Otherwise, the style of the C code looks fine to my untrained eye.

Actually, shouldn't it return text rather than varchar?

* Does it really require four functions to do its work? Might there be some way to use the array_agg() C functions and then just a different final function to turn it into a string (using the internal array_to_string() function, perhaps)? I'm not at all sure about it, but given how little code was required to create the same basic functionality in SQL, I'm surprised that the C implementation requires four functions (accumStringResult(), listagg1_transfn(), listagg2_transfn(), and listagg_finalfn()). Maybe they're required to make it fast and avoid the overhead of an array?

* No compiler warnings, I never made it crash, good comments, does what it says on the tin. I doubt that there are any portability issues, as the code seems to use standard PostgreSQL internal macros and functions.

Architecture Review
-------------------

* No dependencies, things seem to make sense overall, notwithstanding my questions in the Coding Review.

Review Review
-------------

The only thing I haven't covered so far is the name. I agree with Tom's assertion that the name is awful. Sure there may be a precedent in Oracle, but I hardly find that convincing (some of the big corporations seem to do a really shitty job naming things in their APIs). Given that we already have array_agg(), what about simply concat_agg(), as Tom (wincingly, I grant you) suggested? Or string_agg()?

My bike shed is puce.

Attached is a new patch with the changed OIDs and an added phrase in the documentation that indicates that the second argument can be used to separate the concatenated values.

Best,

David