string_agg delimiter having no effect with order by

On 4 August 2010 10:36, Thom Brown <thom@linux.com> wrote:

I'd like to report a potential bug (or just my misunderstanding), but
I couldn't find any mention in the TODO or on the mailing list.

I'm using PostgreSQL 9.0 beta 3 on Gentoo x64 (sorry, I don't have
beta 4 yet).  I attempted to use string_agg to get values into a
comma-separated list as follows.

test=# create table agg_test (
id serial,
thing integer,
stuff text);
NOTICE:  CREATE TABLE will create implicit sequence "agg_test_id_seq"
for serial column "agg_test.id"
CREATE TABLE

test=# insert into agg_test (thing, stuff) values (1,'meow'),(1,'bark');
INSERT 0 2

test=# select thing, string_agg(stuff order by stuff, ',') from
agg_test group by thing;
 thing | string_agg
-------+------------
    1 | barkmeow
(1 row)

test=# select thing, string_agg(stuff order by thing, ',') from
agg_test group by thing;
 thing | string_agg
-------+------------
    1 | meowbark
(1 row)

As you can see, the output of string_agg isn't delimited.  But if I
remove order by, it works:

test=# select thing, string_agg(stuff, ',') from agg_test group by thing;
 thing | string_agg
-------+------------
    1 | meow,bark
(1 row)

The reason I expect this to work is because of what is stated in the
documentation: http://www.postgresql.org/docs/9.0/static/functions-aggregate.html

"This ordering is unspecified by default, but can be controlled by
writing an ORDER BY clause within the aggregate call, as shown in
Section 4.2.7. "

Thanks

--
Thom Brown
Registered Linux user: #516935

I also notice that there are no regression tests for use of string_agg
with both ORDER BY and a delimiter.

Thom

On 4 August 2010 10:44, Thom Brown <thom@linux.com> wrote:

On 4 August 2010 10:36, Thom Brown <thom@linux.com> wrote:

I'd like to report a potential bug (or just my misunderstanding), but
I couldn't find any mention in the TODO or on the mailing list.

I'm using PostgreSQL 9.0 beta 3 on Gentoo x64 (sorry, I don't have
beta 4 yet).  I attempted to use string_agg to get values into a
comma-separated list as follows.

test=# create table agg_test (
id serial,
thing integer,
stuff text);
NOTICE:  CREATE TABLE will create implicit sequence "agg_test_id_seq"
for serial column "agg_test.id"
CREATE TABLE

test=# insert into agg_test (thing, stuff) values (1,'meow'),(1,'bark');
INSERT 0 2

test=# select thing, string_agg(stuff order by stuff, ',') from
agg_test group by thing;
 thing | string_agg
-------+------------
    1 | barkmeow
(1 row)

test=# select thing, string_agg(stuff order by thing, ',') from
agg_test group by thing;
 thing | string_agg
-------+------------
    1 | meowbark
(1 row)

As you can see, the output of string_agg isn't delimited.  But if I
remove order by, it works:

test=# select thing, string_agg(stuff, ',') from agg_test group by thing;
 thing | string_agg
-------+------------
    1 | meow,bark
(1 row)

The reason I expect this to work is because of what is stated in the
documentation: http://www.postgresql.org/docs/9.0/static/functions-aggregate.html

"This ordering is unspecified by default, but can be controlled by
writing an ORDER BY clause within the aggregate call, as shown in
Section 4.2.7. "

Thanks

--
Thom Brown
Registered Linux user: #516935

I also notice that there are no regression tests for use of string_agg
with both ORDER BY and a delimiter.

Thom

Actually, this rings a bell. I think this may have been raised
before, something to do with the delimiter being accepted as one of
the order by values. If this isn't really a bug, could someone
mention it in the docs somewhere?

Thom

On Wed, Aug 4, 2010 at 6:03 AM, Thom Brown <thom@linux.com> wrote:

Actually, this rings a bell.  I think this may have been raised
before, something to do with the delimiter being accepted as one of
the order by values.  If this isn't really a bug, could someone
mention it in the docs somewhere?

Oh, yeah. I guess you need this:

select thing, string_agg(stuff, ',' order by stuff) from agg_test
group by thing;

Rather than this:

select thing, string_agg(stuff order by stuff, ',') from agg_test
group by thing;

It's all kinds of not obvious to me what the second one is supposed to
mean, but I remember this was discussed before. Perhaps we need a
<note> somewhere about multi-argument aggregates.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On 4 August 2010 14:04, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Aug 4, 2010 at 6:03 AM, Thom Brown <thom@linux.com> wrote:

Actually, this rings a bell.  I think this may have been raised
before, something to do with the delimiter being accepted as one of
the order by values.  If this isn't really a bug, could someone
mention it in the docs somewhere?

Oh, yeah.  I guess you need this:

select thing, string_agg(stuff, ',' order by stuff) from agg_test
group by thing;

Rather than this:

select thing, string_agg(stuff order by stuff, ',') from agg_test
group by thing;

It's all kinds of not obvious to me what the second one is supposed to
mean, but I remember this was discussed before.  Perhaps we need a
<note> somewhere about multi-argument aggregates.

Yes, that works with the order clause. That's really weird! It looks
like part of the delimiter parameter, and that's undocumented, or at
least impossible to gleen from the documentation.

This should be clarified as it looks like having ORDER BY *or* a
delimiter is supported, but not both. It's horribly unintuitive!
This is one of the very few cases where MySQL's version actually makes
more sense.

Thom

2010/8/4 Thom Brown <thom@linux.com>:

On 4 August 2010 14:04, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Aug 4, 2010 at 6:03 AM, Thom Brown <thom@linux.com> wrote:

Actually, this rings a bell.  I think this may have been raised
before, something to do with the delimiter being accepted as one of
the order by values.  If this isn't really a bug, could someone
mention it in the docs somewhere?

Oh, yeah.  I guess you need this:

select thing, string_agg(stuff, ',' order by stuff) from agg_test
group by thing;

Rather than this:

select thing, string_agg(stuff order by stuff, ',') from agg_test
group by thing;

It's all kinds of not obvious to me what the second one is supposed to
mean, but I remember this was discussed before.  Perhaps we need a
<note> somewhere about multi-argument aggregates.

Yes, that works with the order clause.  That's really weird!  It looks
like part of the delimiter parameter, and that's undocumented, or at
least impossible to gleen from the documentation.

This should be clarified as it looks like having ORDER BY *or* a
delimiter is supported, but not both.  It's horribly unintuitive!
This is one of the very few cases where MySQL's version actually makes
more sense.

this goes from ANSI SQL standard :( - I agree, this isn't intuitive
and pg can do better diagnostic now. But it has a sense. ORDER BY
hasn't sense for one parameter - only for complete function, so is
wrong to write ORDER BY over a some interesting parameter

Regards

Pavel Stehule

On 4 August 2010 14:24, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2010/8/4 Thom Brown <thom@linux.com>:

On 4 August 2010 14:04, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Aug 4, 2010 at 6:03 AM, Thom Brown <thom@linux.com> wrote:

Actually, this rings a bell.  I think this may have been raised
before, something to do with the delimiter being accepted as one of
the order by values.  If this isn't really a bug, could someone
mention it in the docs somewhere?

Oh, yeah.  I guess you need this:

select thing, string_agg(stuff, ',' order by stuff) from agg_test
group by thing;

Rather than this:

select thing, string_agg(stuff order by stuff, ',') from agg_test
group by thing;

It's all kinds of not obvious to me what the second one is supposed to
mean, but I remember this was discussed before.  Perhaps we need a
<note> somewhere about multi-argument aggregates.

Yes, that works with the order clause.  That's really weird!  It looks
like part of the delimiter parameter, and that's undocumented, or at
least impossible to gleen from the documentation.

This should be clarified as it looks like having ORDER BY *or* a
delimiter is supported, but not both.  It's horribly unintuitive!
This is one of the very few cases where MySQL's version actually makes
more sense.

this goes from ANSI SQL standard :( - I agree, this isn't intuitive
and pg can do better diagnostic now. But it has a sense. ORDER BY
hasn't sense for one parameter - only for complete function, so is
wrong to write ORDER BY over a some interesting parameter

Regards

Pavel Stehule

So really, should the documentation be changed from:

string_agg(expression [, delimiter ] )

to

string_agg(expression [, delimiter ] [ GROUP BY expression [, ...] ] )

?

--
Thom Brown
Registered Linux user: #516935

2010/8/4 Thom Brown <thom@linux.com>:

On 4 August 2010 14:24, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2010/8/4 Thom Brown <thom@linux.com>:

On 4 August 2010 14:04, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Aug 4, 2010 at 6:03 AM, Thom Brown <thom@linux.com> wrote:

Actually, this rings a bell.  I think this may have been raised
before, something to do with the delimiter being accepted as one of
the order by values.  If this isn't really a bug, could someone
mention it in the docs somewhere?

Oh, yeah.  I guess you need this:

select thing, string_agg(stuff, ',' order by stuff) from agg_test
group by thing;

Rather than this:

select thing, string_agg(stuff order by stuff, ',') from agg_test
group by thing;

It's all kinds of not obvious to me what the second one is supposed to
mean, but I remember this was discussed before.  Perhaps we need a
<note> somewhere about multi-argument aggregates.

Yes, that works with the order clause.  That's really weird!  It looks
like part of the delimiter parameter, and that's undocumented, or at
least impossible to gleen from the documentation.

This should be clarified as it looks like having ORDER BY *or* a
delimiter is supported, but not both.  It's horribly unintuitive!
This is one of the very few cases where MySQL's version actually makes
more sense.

this goes from ANSI SQL standard :( - I agree, this isn't intuitive
and pg can do better diagnostic now. But it has a sense. ORDER BY
hasn't sense for one parameter - only for complete function, so is
wrong to write ORDER BY over a some interesting parameter

Regards

Pavel Stehule

So really, should the documentation be changed from:

string_agg(expression [, delimiter ] )

to

string_agg(expression [, delimiter ] [ GROUP BY expression [, ...] ] )

This syntax is available for all aggregate functions - this feature
isn't specific for string_agg

but there can be more descriptive example.

Regards

Pavel

Robert Haas <robertmhaas@gmail.com> writes:

Oh, yeah. I guess you need this:

select thing, string_agg(stuff, ',' order by stuff) from agg_test
group by thing;

Rather than this:

select thing, string_agg(stuff order by stuff, ',') from agg_test
group by thing;

It's all kinds of not obvious to me what the second one is supposed to
mean, but I remember this was discussed before. Perhaps we need a
<note> somewhere about multi-argument aggregates.

Done:

+    <para>
+     When dealing with multiple-argument aggregate functions, note that the
+     <literal>ORDER BY</> clause goes after all the aggregate arguments.
+     For example, this:
+ <programlisting>
+ SELECT string_agg(a, ',' ORDER BY a) FROM table;
+ </programlisting>
+     not this:
+ <programlisting>
+ SELECT string_agg(a ORDER BY a, ',') FROM table;  -- not what you want
+ </programlisting>
+     The latter syntax will be accepted, but <literal>','</> will be
+     treated as a (useless) sort key.
+    </para>

regards, tom lane

On Wed, Aug 4, 2010 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Oh, yeah.  I guess you need this:

select thing, string_agg(stuff, ',' order by stuff) from agg_test
group by thing;

Rather than this:

select thing, string_agg(stuff order by stuff, ',') from agg_test
group by thing;

It's all kinds of not obvious to me what the second one is supposed to
mean, but I remember this was discussed before.  Perhaps we need a
<note> somewhere about multi-argument aggregates.

Done:

+    <para>
+     When dealing with multiple-argument aggregate functions, note that the
+     <literal>ORDER BY</> clause goes after all the aggregate arguments.
+     For example, this:
+ <programlisting>
+ SELECT string_agg(a, ',' ORDER BY a) FROM table;
+ </programlisting>
+     not this:
+ <programlisting>
+ SELECT string_agg(a ORDER BY a, ',') FROM table;  -- not what you want
+ </programlisting>
+     The latter syntax will be accepted, but <literal>','</> will be
+     treated as a (useless) sort key.
+    </para>

Oh, right, that's what it's supposed to mean. Thanks for adding this.
I suppose this confusion is only possible because string_agg has both
a one-argument and a two-argument form.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas@gmail.com> writes:

I suppose this confusion is only possible because string_agg has both
a one-argument and a two-argument form.

Right, or at least that's what allows the mistake to go through without
reporting any error.

regards, tom lane

On Wed, Aug 4, 2010 at 12:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

 I suppose this confusion is only possible because string_agg has both
a one-argument and a two-argument form.

Right, or at least that's what allows the mistake to go through without
reporting any error.

No, that's what lets the correct form go through without reporting any error.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Aug 4, 2010 at 12:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I suppose this confusion is only possible because string_agg has both
a one-argument and a two-argument form.

Right, or at least that's what allows the mistake to go through without
reporting any error.

No, that's what lets the correct form go through without reporting any error.

Really? IMO the reason Thom had a problem was he thought he was
invoking the two-argument form of string_agg, but he was really
invoking the one-argument form.

If we were a bit earlier in the 9.0 cycle I would suggest that this
confusion is a sufficient reason to drop the one-argument form of
string_agg. It's too late now though.

regards, tom lane

On Wed, Aug 4, 2010 at 1:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Aug 4, 2010 at 12:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I suppose this confusion is only possible because string_agg has both
a one-argument and a two-argument form.

Right, or at least that's what allows the mistake to go through without
reporting any error.

No, that's what lets the correct form go through without reporting any error.

Really?  IMO the reason Thom had a problem was he thought he was
invoking the two-argument form of string_agg, but he was really
invoking the one-argument form.

I had my head tilted a slightly different way, but, yes.

If we were a bit earlier in the 9.0 cycle I would suggest that this
confusion is a sufficient reason to drop the one-argument form of
string_agg.  It's too late now though.

Agreed on both points.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Wed, Aug 4, 2010 at 11:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:

If we were a bit earlier in the 9.0 cycle I would suggest that this
confusion is a sufficient reason to drop the one-argument form of
string_agg.  It's too late now though.

FWIW I think we can still change it. Isn't this type of issue part
of what beta is for? If we were in RC that would be a different story
:)

Alex Hunsaker <badalex@gmail.com> wrote:

On Wed, Aug 4, 2010 at 11:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:

If we were a bit earlier in the 9.0 cycle I would suggest that
this confusion is a sufficient reason to drop the one-argument
form of string_agg. It's too late now though.

FWIW I think we can still change it. Isn't this type of issue
part of what beta is for? If we were in RC that would be a
different story

I like to think I'm pretty serious about controlling scope creep to
prevent a release dragging out, but this one seems like beta testing
uncovered a flaw in new code for the release. In my book, that
makes it fair game to balance the risk of breaking things by
changing it now against the problems we'll have long term if we
leave it alone. I'm not sure if that was the basis of saying it was
too late, or some other consideration.

-Kevin

Alex Hunsaker <badalex@gmail.com> writes:

On Wed, Aug 4, 2010 at 11:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:

If we were a bit earlier in the 9.0 cycle I would suggest that this
confusion is a sufficient reason to drop the one-argument form of
string_agg. It's too late now though.

FWIW I think we can still change it. Isn't this type of issue part
of what beta is for? If we were in RC that would be a different story
:)

Well, it'd take an initdb to get rid of it. In the past we've avoided
forcing initdb post-beta1 unless it was Really Necessary. OTOH, we seem
to be in the mode of encouraging beta testers to test pg_upgrade, so
maybe that concern isn't worth much at the moment.

I am right, am I not, in thinking that we invented string_agg out of
whole cloth? I don't see it in SQL:2008. If there is a compatibility-
with-other-products reason to support the one-argument form, that would
be a consideration here. I don't see a whole lot of functionality gain
from having the one-argument form, though.

BTW, as far as I can tell from checking in the system catalogs,
there are no other built-in aggregates that come in
differing-numbers-of-arguments variants. So string_agg is the only
one presenting this hazard.

regards, tom lane

On Wed, Aug 4, 2010 at 13:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alex Hunsaker <badalex@gmail.com> writes:

On Wed, Aug 4, 2010 at 11:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:

If we were a bit earlier in the 9.0 cycle I would suggest that this
confusion is a sufficient reason to drop the one-argument form of
string_agg. It's too late now though.

FWIW I think we can still change it.   Isn't this type of issue part
of what beta is for?  If we were in RC that would be a different story
:)

Well, it'd take an initdb to get rid of it.

I think forcing an initdb might be more trouble than this wart is worth.

In the past we've avoided
forcing initdb post-beta1 unless it was Really Necessary.  OTOH, we seem
to be in the mode of encouraging beta testers to test pg_upgrade, so
maybe that concern isn't worth much at the moment.

I have one or two 9.0-beta databases, a forced initdb would defiantly
motivate me to try pg_upgrade :). To me, the question is are we
planning on releasing a new beta anyway? Maybe its worth it then. If
we were planning on going RC after this last beta (and I dont think we
were?), I agree with Kevin, its not something worth pushing the
release 9.0 for. By that I mean I assume if we force an initdb that
we would want to do another beta regardless.

Either way, I don't have strong feelings on this other than if we dont
fix it now when will we? Maybe we will get "lucky" and someone will
find an issue that we have to initdb for anyways :).

On Wed, Aug 4, 2010 at 3:25 PM, Alex Hunsaker <badalex@gmail.com> wrote:

I think forcing an initdb might be more trouble than this wart is worth.

+1. I would not make this change unless we have to force an initdb
anyway. And I really hope we don't, because I'm sort of hoping the
next 9.0 release will be rc1.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Alex Hunsaker <badalex@gmail.com> writes:

Either way, I don't have strong feelings on this other than if we dont
fix it now when will we?

Well, we won't. If 9.0 ships with both forms of string_agg, we're stuck
with it IMO. It's not exactly a bug, so I won't cry if that's how
things go; but it is striking that already two different people have
gotten confused enough to file bug reports because of this. If we don't
pull the one-argument form then I think we can look forward to many more
of those in future years.

regards, tom lane