Review: listagg aggregate

Hello

2010/1/22 David E. Wheeler <david@kineticode.com>:

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.

can I help with it, please. My English is terrible.

* 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.

array user functions are used more time in pg core. The complexity of
array functions are much higher, so I don't think we need special
file.

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

I'll recheck it. I am sure so all parameters should be a text.

* 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)?

We can, but it isn't good way. Processing of arrays is little bit more
expensive then processing plain text. It is reason why listagg is
faster, than your custom aggregate. More, the final function could be
faster - the content is final.

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?

It normal for aggregate functions. We need more transfn function,
because we need two two variant: listagg(col), listagg(col, sep). Our
coding guidlines doesn't advice share C functions - but these
functions are +/- wrapper for accumStringResult - so there is zero
overhead.

* 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()?

I don't think. When we have function, with same parameters, same
behave like some Oracle function, then I am strongly prefer Oracle
name. I don't see any benefit from different name. It can only confuse
developers and add the trable to people who porting applications.

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,

Thank you very much
Regards
Pavel

On Jan 24, 2010, at 1:19 AM, Pavel Stehule wrote:

can I help with it, please. My English is terrible.

Yes, I added a bit in the patch I submitted.

array user functions are used more time in pg core. The complexity of
array functions are much higher, so I don't think we need special
file.

Okay. Should have tried it in PL/pgSQL then, perhaps.

I'll recheck it. I am sure so all parameters should be a text.

Probably shouldn't go into varchar.c then, yes?

We can, but it isn't good way. Processing of arrays is little bit more
expensive then processing plain text. It is reason why listagg is
faster, than your custom aggregate. More, the final function could be
faster - the content is final.

Understood.

It normal for aggregate functions. We need more transfn function,
because we need two two variant: listagg(col), listagg(col, sep). Our
coding guidlines doesn't advice share C functions - but these
functions are +/- wrapper for accumStringResult - so there is zero
overhead.

Ah, okay, it's because of the second argument. Now I understand.

I don't think. When we have function, with same parameters, same
behave like some Oracle function, then I am strongly prefer Oracle
name. I don't see any benefit from different name. It can only confuse
developers and add the trable to people who porting applications.

Meh. If the name is terrible, we don't have to use it, and it's easy enough to create an alias in SQL for those who need it.

Best,

David

On Fri, 2010-01-22 at 11:14 -0800, David E.Wheeler wrote:

No performance issues

ISTM that this class of function is inherently dangerous performance
wise.

* It looks incredibly easy to construct enormous lists. We should test
the explosion limit of this to see how it is handled. Perhaps we need
some parameter limits to control that, depending upon results.

* Optimizer doesn't consider whether the result type of an aggregate get
bigger as the aggregate processes more rows. If we're adding this
function we should give some thought in that area also, or at least a
comment to note that it can and will cause the optimizer problems in
complex queries.

--
Simon Riggs www.2ndQuadrant.com

2010/1/24 Simon Riggs <simon@2ndquadrant.com>:

On Fri, 2010-01-22 at 11:14 -0800, David E.Wheeler wrote:

No performance issues

ISTM that this class of function is inherently dangerous performance
wise.

there is potencial risk, but this risk isn't new. The almost all what
you say is true for array aggregates.

* It looks incredibly easy to construct enormous lists. We should test
the explosion limit of this to see how it is handled. Perhaps we need
some parameter limits to control that, depending upon results.

There are no limit for generating large values - like bytea, xml, or
text. There are not limit for array_accum or array(query). So, I don't
think we need some special mechanism for listagg. If somebody will
generate too large a value, then he will get "out of memory"
exception.

* Optimizer doesn't consider whether the result type of an aggregate get
bigger as the aggregate processes more rows. If we're adding this
function we should give some thought in that area also, or at least a
comment to note that it can and will cause the optimizer problems in
complex queries.

this is true, but this isn't some new. array_accum working well
without optimizer problems.

Simon Riggs <simon@2ndQuadrant.com> writes:

On Fri, 2010-01-22 at 11:14 -0800, David E.Wheeler wrote:

No performance issues

ISTM that this class of function is inherently dangerous performance
wise.

* It looks incredibly easy to construct enormous lists. We should test
the explosion limit of this to see how it is handled. Perhaps we need
some parameter limits to control that, depending upon results.

* Optimizer doesn't consider whether the result type of an aggregate get
bigger as the aggregate processes more rows. If we're adding this
function we should give some thought in that area also, or at least a
comment to note that it can and will cause the optimizer problems in
complex queries.

We have that problem already with array_agg(), and I don't recall many
complaints about it. It might be worth worrying about at some point,
but I don't think it's reasonable to insist that it be fixed before
any more such aggregates are created.

I agree that testing-to-failure would be a good idea just to be sure it
fails cleanly.

regards, tom lane

2010/1/24 David E. Wheeler <david@kineticode.com>:

On Jan 24, 2010, at 1:19 AM, Pavel Stehule wrote:

can I help with it, please. My English is terrible.

Yes, I added a bit in the patch I submitted.

array user functions are used more time in pg core. The complexity of
array functions are much higher, so I don't think we need special
file.

Okay. Should have tried it in PL/pgSQL then, perhaps.

:) - I'll look on it again.

I'll recheck it. I am sure so all parameters should be a text.

Probably shouldn't go into varchar.c then, yes?

We can, but it isn't good way. Processing of arrays is little bit more
expensive then processing plain text. It is reason why listagg is
faster, than your custom aggregate. More, the final function could be
faster - the content is final.

Understood.

It normal for aggregate functions. We need more transfn function,
because we need two two variant: listagg(col), listagg(col, sep). Our
coding guidlines doesn't advice share C functions - but these
functions are +/- wrapper for accumStringResult - so there is zero
overhead.

Ah, okay, it's because of the second argument. Now I understand.

I don't think. When we have function, with same parameters, same
behave like some Oracle function, then I am strongly prefer Oracle
name. I don't see any benefit from different name. It can only confuse
developers and add the trable to people who porting applications.

Meh. If the name is terrible, we don't have to use it, and it's easy enough to create an alias in SQL for those who need it.

The "list" is common name for this content - it is usual in Microsoft
SQL Server, it is usual in Oracle. Maybe we can vote about the name

Regards
Pavel Stehule

I don't think. When we have function, with same parameters, same
behave like some Oracle function, then I am strongly prefer Oracle
name. I don't see any benefit from different name. It can only confuse
developers and add the trable to people who porting applications.

Meh. If the name is terrible, we don't have to use it, and it's easy enough to create an alias in SQL for those who need it.

The corresponding function in Oracle is called wm_concat. In MySQL its
called group_concat. I don't believe DB2 or SQL Server have built in
equivalents. The Oracle name isn't really an option ("wm' stands for
workspace manager)

I think listagg or string_agg would be the most appropriate names. Oh
and before Oracle had wm_concat, Tom Kyte wrote a function called stragg
that was pretty popular.

Scott

On sön, 2010-01-24 at 21:29 -0800, Scott Bailey wrote:

I think listagg or string_agg would be the most appropriate names. Oh
and before Oracle had wm_concat, Tom Kyte wrote a function called
stragg that was pretty popular.

Well,

xmlagg -> concatenates values to form xml datum
array_agg -> concatenates values to form array datum
??? -> concatenates values to form string datum

So it's pretty clear that listagg does not fit into this scheme.

2010/1/25 Peter Eisentraut <peter_e@gmx.net>:

On sön, 2010-01-24 at 21:29 -0800, Scott Bailey wrote:

I think listagg or string_agg would be the most appropriate names. Oh
and before Oracle had wm_concat, Tom Kyte wrote a function called
stragg that was pretty popular.

Well,

xmlagg -> concatenates values to form xml datum
array_agg -> concatenates values to form array datum
??? -> concatenates values to form string datum

So it's pretty clear that listagg does not fit into this scheme.

when you define list as text domain, then this the name is correct.

searching "list sql string" on google. You can see, so "list" is usually used.

Regards
Pavel

Pavel Stehule <pavel.stehule@gmail.com> writes:

2010/1/25 Peter Eisentraut <peter_e@gmx.net>:

xmlagg -> concatenates values to form xml datum
array_agg -> concatenates values to form array datum
??? -> concatenates values to form string datum

So it's pretty clear that listagg does not fit into this scheme.

when you define list as text domain, then this the name is correct.

IOW, if you define away the problem then there's no problem?

I agree that "list" is a terrible choice of name here. "string_agg"
seemed reasonable and in keeping with the standardized "array_agg".

regards, tom lane

2010/1/25 Tom Lane <tgl@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.com> writes:

2010/1/25 Peter Eisentraut <peter_e@gmx.net>:

xmlagg -> concatenates values to form xml datum
array_agg -> concatenates values to form array datum
??? -> concatenates values to form string datum

So it's pretty clear that listagg does not fit into this scheme.

when you define list as text domain, then this the name is correct.

IOW, if you define away the problem then there's no problem?

I agree that "list" is a terrible choice of name here.  "string_agg"
seemed reasonable and in keeping with the standardized "array_agg".

I am not happy, I thing so we do bigger chaos then it is. But it
hasn't progress. So I agree with name string_agg. In this case isn't a
problem rename this function if somebody would.

I'll send patch over hour.

regards
Pavel Stehule

2010/1/25 Tom Lane <tgl@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.com> writes:

2010/1/25 Peter Eisentraut <peter_e@gmx.net>:

xmlagg -> concatenates values to form xml datum
array_agg -> concatenates values to form array datum
??? -> concatenates values to form string datum

So it's pretty clear that listagg does not fit into this scheme.

when you define list as text domain, then this the name is correct.

IOW, if you define away the problem then there's no problem?

I agree that "list" is a terrible choice of name here.  "string_agg"
seemed reasonable and in keeping with the standardized "array_agg".

actualised patch - the name is string_agg

regards
Pavel Stehule

2010/1/24 Tom Lane <tgl@sss.pgh.pa.us>:

Simon Riggs <simon@2ndQuadrant.com> writes:

On Fri, 2010-01-22 at 11:14 -0800, David E.Wheeler wrote:

No performance issues

ISTM that this class of function is inherently dangerous performance
wise.

* It looks incredibly easy to construct enormous lists. We should test
the explosion limit of this to see how it is handled. Perhaps we need
some parameter limits to control that, depending upon results.

* Optimizer doesn't consider whether the result type of an aggregate get
bigger as the aggregate processes more rows. If we're adding this
function we should give some thought in that area also, or at least a
comment to note that it can and will cause the optimizer problems in
complex queries.

We have that problem already with array_agg(), and I don't recall many
complaints about it.  It might be worth worrying about at some point,
but I don't think it's reasonable to insist that it be fixed before
any more such aggregates are created.

I agree that testing-to-failure would be a good idea just to be sure it
fails cleanly.

postgres=# \timing
Timing is on.
postgres=# select
pg_size_pretty(length(string_agg('012345678901234567890'::text,',')))
from generate_series(1,10000000) g(i);
pg_size_pretty
----------------
210 MB
(1 row)

Time: 5831,218 ms
postgres=# select
pg_size_pretty(length(string_agg('012345678901234567890'::text,',')))
from generate_series(1,50000000) g(i);
^[^[ERROR: out of memory
DETAIL: Cannot enlarge string buffer containing 1073741812 bytes by
21 more bytes.
postgres=#

I thing, so 210 MB is more then is necessary :)

Regards
Pavel Stehule

On Jan 25, 2010, at 2:09 AM, Peter Eisentraut wrote:

xmlagg -> concatenates values to form xml datum
array_agg -> concatenates values to form array datum
??? -> concatenates values to form string datum

concat_agg().

David

On Jan 25, 2010, at 6:12 AM, Pavel Stehule wrote:

I am not happy, I thing so we do bigger chaos then it is. But it
hasn't progress. So I agree with name string_agg. In this case isn't a
problem rename this function if somebody would.

Could you not use CREATE AGGREGATE to create an alias named listagg if you needed it? Or are we limited to a single argument in that case?

David

On Mon, Jan 25, 2010 at 2:27 PM, David E. Wheeler <david@kineticode.com> wrote:

On Jan 25, 2010, at 2:09 AM, Peter Eisentraut wrote:

xmlagg -> concatenates values to form xml datum
array_agg -> concatenates values to form array datum
??? -> concatenates values to form string datum

concat_agg().

I like that one...

...Robert

2010/1/25 David E. Wheeler <david@kineticode.com>:

On Jan 25, 2010, at 6:12 AM, Pavel Stehule wrote:

I am not happy, I thing so we do bigger chaos then it is. But it
hasn't progress. So I agree with name string_agg. In this case isn't a
problem rename this function if somebody would.

Could you not use CREATE AGGREGATE to create an alias named listagg if you needed it? Or are we limited to a single argument in that case?

we can, but without one common known name we will have a propriety name.

Pavel

2010/1/25 Robert Haas <robertmhaas@gmail.com>:

On Mon, Jan 25, 2010 at 2:27 PM, David E. Wheeler <david@kineticode.com> wrote:

On Jan 25, 2010, at 2:09 AM, Peter Eisentraut wrote:

xmlagg -> concatenates values to form xml datum
array_agg -> concatenates values to form array datum
??? -> concatenates values to form string datum

concat_agg().

I like that one...

why is concat_agg better than listagg ?

Pavel

On Jan 25, 2010, at 23:14, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

why is concat_agg better than listagg ?

Because it's an aggregate that cocatenates values. It's not an
aggregate that lists things. I also like concat_agg better than
string_agg because it's not limited to acting on strings.

Best,

David