string_agg delimiter having no effect with order by

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

On 4 August 2010 10:36, Thom Brown <thom(at)linux(dot)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(at)linux(dot)com> wrote:
> On 4 August 2010 10:36, Thom Brown <thom(at)linux(dot)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(at)linux(dot)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(at)gmail(dot)com> wrote:
> On Wed, Aug 4, 2010 at 6:03 AM, Thom Brown <thom(at)linux(dot)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(at)linux(dot)com>:
> On 4 August 2010 14:04, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Aug 4, 2010 at 6:03 AM, Thom Brown <thom(at)linux(dot)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

>
> Thom
>
> --
> Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs
>

On 4 August 2010 14:24, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2010/8/4 Thom Brown <thom(at)linux(dot)com>:
>> On 4 August 2010 14:04, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Wed, Aug 4, 2010 at 6:03 AM, Thom Brown <thom(at)linux(dot)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(at)linux(dot)com>:
> On 4 August 2010 14:24, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> 2010/8/4 Thom Brown <thom(at)linux(dot)com>:
>>> On 4 August 2010 14:04, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> On Wed, Aug 4, 2010 at 6:03 AM, Thom Brown <thom(at)linux(dot)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
>
> ?
>
> --
> Thom Brown
> Registered Linux user: #516935
>

Robert Haas <robertmhaas(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)com> writes:
> On Wed, Aug 4, 2010 at 12:44 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Aug 4, 2010 at 12:44 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> wrote:
> On Wed, Aug 4, 2010 at 11:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
> On Wed, Aug 4, 2010 at 11:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alex Hunsaker <badalex(at)gmail(dot)com> writes:
>> On Wed, Aug 4, 2010 at 11:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)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(at)gmail(dot)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

On 4 August 2010 20:25, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> On Wed, Aug 4, 2010 at 13:11, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Alex Hunsaker <badalex(at)gmail(dot)com> writes:
>>> On Wed, Aug 4, 2010 at 11:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 :).
>

I think it should be left exactly how it is. It only needed
clarification in the documentation to explain its usage for the
scenario in question, and probably a couple entries in the regression
tests as they're lacking at the moment.

I wish I had held back on mentioning it as I remembered later that
this has already been discussed to a degree, and I'd probably have
kept my mouth shut upon recalling it.

--
Thom Brown
Registered Linux user: #516935

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Aug 4, 2010 at 3:25 PM, Alex Hunsaker <badalex(at)gmail(dot)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.

Hm? I don't think that an initdb here would have any impact on whether
we can call the next drop RC1 or not. We're talking about removing a
single built-in entry in pg_proc --- it's one of the safest changes we
could possibly make. The only argument I can see against it is not
wanting to force beta testers through an initdb. But it seems like that
might actually be a positive thing not a negative one, in this cycle,
because we're trying to get test coverage on pg_upgrade.

regards, tom lane

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

If it's causing bugs, drop it now. If we include it in 9.0, we're stuck
with it for years.

I'm OK with forcing an initDB for RC1.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> If it's causing bugs, drop it now. If we include it in 9.0, we're stuck
> with it for years.

Well, it's causing bug reports, which is not exactly the same thing as bugs.
But yeah, I'm thinking we should get rid of it.

regards, tom lane

04.Ağu.2010 tarihinde 22:44 saatinde, Josh Berkus <josh(at)agliodbs(dot)com>
şunları yazdı:

> I'm OK with forcing an initDB for RC1.

I think beta5 will be a better choice than RC 1 here.
--
Devrim GÜNDÜZ
PostgreSQL DBA @ Akinon/Markafoni, Red Hat Certified Engineer
devrim~gunduz.org, devrim~PostgreSQL.org, devrim.gunduz~linux.org.tr
http://www.gunduz.org Twitter: http://twitter.com/devrimgunduz

On Wed, Aug 4, 2010 at 13:42, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Aug 4, 2010 at 3:25 PM, Alex Hunsaker <badalex(at)gmail(dot)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.
>
> Hm?  I don't think that an initdb here would have any impact on whether
> we can call the next drop RC1 or not.  We're talking about removing a
> single built-in entry in pg_proc --- it's one of the safest changes we
> could possibly make.

Great, I was afraid people would want another beta if we forced an
initdb. So a hearty +1 for fixing it and not doing another beta
(pending other bugs obviously).

> Great, I was afraid people would want another beta if we forced an
> initdb. So a hearty +1 for fixing it and not doing another beta
> (pending other bugs obviously).

And, btw, there has been a lot of testing of pg_upgrade due to the
initdbs and otherwise. I think 9.0 is going to have a pretty darned
solid pg_upgrade because of it.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

On 4 August 2010 20:58, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>
>> Great, I was afraid people would want another beta if we forced an
>> initdb.  So a hearty +1 for fixing it and not doing another beta
>> (pending other bugs obviously).
>
> And, btw, there has been a lot of testing of pg_upgrade due to the
> initdbs and otherwise.  I think 9.0 is going to have a pretty darned
> solid pg_upgrade because of it.
>

Leave my name off the commit comment then ;) People who have been
waiting for this will burn me as a heretical witch... er.. man
witch... warlock?

--
Thom Brown
Registered Linux user: #516935

On Wed, Aug 04, 2010 at 09:02:43PM +0100, Thom Brown wrote:
> On 4 August 2010 20:58, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> >
> >> Great, I was afraid people would want another beta if we forced
> >> an initdb.  So a hearty +1 for fixing it and not doing another
> >> beta (pending other bugs obviously).
> >
> > And, btw, there has been a lot of testing of pg_upgrade due to the
> > initdbs and otherwise.  I think 9.0 is going to have a pretty
> > darned solid pg_upgrade because of it.
> >
>
> Leave my name off the commit comment then ;) People who have been
> waiting for this will burn me as a heretical witch... er.. man
> witch... warlock?

Witch.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, Aug 4, 2010 at 3:11 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alex Hunsaker <badalex(at)gmail(dot)com> writes:
>> On Wed, Aug 4, 2010 at 11:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 vote fix it. This is going to be a high travel function, and should be right.

merlin

I wrote:
> Hm? I don't think that an initdb here would have any impact on whether
> we can call the next drop RC1 or not. We're talking about removing a
> single built-in entry in pg_proc --- it's one of the safest changes we
> could possibly make.

Well, I forgot that an aggregate involves more than one catalog row ;-).
So it's a bit bigger patch than that, but still pretty small and safe.
See attached.

What we are doing here, IMO, is not just changing string_agg() but
instituting a project policy that we are not going to offer built-in
aggregates with the same names and different numbers of arguments ---
otherwise the problem will come right back. Therefore, the attached
patch adds an opr_sanity regression test that will complain if anyone
tries to add such. Of course, this isn't preventing users from creating
such aggregates, but it's on their own heads to not get confused if they
do.

This policy also implies that we are never going to allow default
arguments for aggregates, or at least never have any built-in ones
that use such a feature.

By my count the following people had offered an opinion on making
this change:
for: tgl, josh, badalex, mmoncure
against: rhaas, thom
Anybody else want to vote, or change their vote after seeing the patch?

regards, tom lane

On 4 August 2010 23:19, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> Hm?  I don't think that an initdb here would have any impact on whether
>> we can call the next drop RC1 or not.  We're talking about removing a
>> single built-in entry in pg_proc --- it's one of the safest changes we
>> could possibly make.
>
> Well, I forgot that an aggregate involves more than one catalog row ;-).
> So it's a bit bigger patch than that, but still pretty small and safe.
> See attached.
>
> What we are doing here, IMO, is not just changing string_agg() but
> instituting a project policy that we are not going to offer built-in
> aggregates with the same names and different numbers of arguments ---
> otherwise the problem will come right back.  Therefore, the attached
> patch adds an opr_sanity regression test that will complain if anyone
> tries to add such.  Of course, this isn't preventing users from creating
> such aggregates, but it's on their own heads to not get confused if they
> do.

Yes, I think that's sensible.

>
> This policy also implies that we are never going to allow default
> arguments for aggregates, or at least never have any built-in ones
> that use such a feature.
>
> By my count the following people had offered an opinion on making
> this change:
>        for: tgl, josh, badalex, mmoncure
>        against: rhaas, thom
> Anybody else want to vote, or change their vote after seeing the patch?
>
>                        regards, tom lane
>
>

I was afraid that the function would be pulled completely, but from
looking at the patch, you're only removing the function with a
single-parameter signature, which is quite innocuous. So I'm "for"
now.
--
Thom Brown
Registered Linux user: #516935

Thom Brown <thom(at)linux(dot)com> writes:
> I was afraid that the function would be pulled completely, but from
> looking at the patch, you're only removing the function with a
> single-parameter signature, which is quite innocuous.

Yes, of course, sorry if I confused anyone. It's the combination of
having both one- and two-argument forms that is the problem. Since
the one-argument form isn't doing anything but offering a rather
useless default, we won't lose functionality if we pull it.

regards, tom lane

On Wed, Aug 04, 2010 at 06:19:49PM -0400, Tom Lane wrote:
> I wrote:
> > Hm? I don't think that an initdb here would have any impact on whether
> > we can call the next drop RC1 or not. We're talking about removing a
> > single built-in entry in pg_proc --- it's one of the safest changes we
> > could possibly make.
>
> Well, I forgot that an aggregate involves more than one catalog row ;-).
> So it's a bit bigger patch than that, but still pretty small and safe.
> See attached.
>
> What we are doing here, IMO, is not just changing string_agg() but
> instituting a project policy that we are not going to offer built-in
> aggregates with the same names and different numbers of arguments ---
> otherwise the problem will come right back. Therefore, the attached
> patch adds an opr_sanity regression test that will complain if anyone
> tries to add such. Of course, this isn't preventing users from creating
> such aggregates, but it's on their own heads to not get confused if they
> do.
>
> This policy also implies that we are never going to allow default
> arguments for aggregates, or at least never have any built-in ones
> that use such a feature.
>
> By my count the following people had offered an opinion on making
> this change:
> for: tgl, josh, badalex, mmoncure
> against: rhaas, thom
> Anybody else want to vote, or change their vote after seeing the patch?

+1 for removing the single-argument version.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, Aug 4, 2010 at 16:33, Thom Brown <thom(at)linux(dot)com> wrote:

> I was afraid that the function would be pulled completely, but from
> looking at the patch, you're only removing the function with a
> single-parameter signature, which is quite innocuous.  So I'm "for"
> now.

Ahh, Now I see why you were worried about people calling you a witch :)

On another note, I do wonder if we could avoid more confusion by just
reordering the arguments. In-fact I bet the original argument
ordering was done precisely so it would match the 1 argument version.

I dunno about anyone else but (a, ',' order by a) just looks weird.

Or in other words, any thoughts on:
select string_agg(delim, expression);

I don't want to stretch this out, but while we are making changes...

Alex Hunsaker <badalex(at)gmail(dot)com> writes:
> I dunno about anyone else but (a, ',' order by a) just looks weird.

I suppose, but aren't you just focusing on the argument being constant?
In the more general case I don't think there's anything unnatural about
this syntax.

> Or in other words, any thoughts on:
> select string_agg(delim, expression);

That looks pretty weird to me anyway, with or without use of ORDER BY.
Nobody would think to write the delimiter first. Usually you put the
"most important" argument first, and no one would see the delimiter
as the most important one.

regards, tom lane

On Wed, Aug 4, 2010 at 7:07 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Or in other words, any thoughts on:
>> select string_agg(delim, expression);
>
> That looks pretty weird to me anyway, with or without use of ORDER BY.
> Nobody would think to write the delimiter first.  Usually you put the
> "most important" argument first, and no one would see the delimiter
> as the most important one.

Well, it would match the syntax of things like perl's join(). But I
think that's probably not enough reason to go fiddling with it.

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

On Wed, Aug 4, 2010 at 17:07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alex Hunsaker <badalex(at)gmail(dot)com> writes:
>> I dunno about anyone else but (a, ',' order by a) just looks weird.
>
> I suppose, but aren't you just focusing on the argument being constant?

Yes.

>> Or in other words, any thoughts on:
>> select string_agg(delim, expression);
>
> That looks pretty weird to me anyway, with or without use of ORDER BY.
> Nobody would think to write the delimiter first.  Usually you put the
> "most important" argument first, and no one would see the delimiter
> as the most important one.

No argument about the most important arg first. I think we have a
difference of opinion on what the important argument is :-)

It might just be my perl upbringing talking, for example you join(',',
...) or split(',', ....) in perl.

Either way *shrug* Im happy to leave it alone.

On Wed, Aug 4, 2010 at 6:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>        for: tgl, josh, badalex, mmoncure
>        against: rhaas, thom
> Anybody else want to vote, or change their vote after seeing the patch?

If we're not regarding this as beta-forcing, I abstain.

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

On Wed, Aug 4, 2010 at 11:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> What we are doing here, IMO, is not just changing string_agg() but
> instituting a project policy that we are not going to offer built-in
> aggregates with the same names and different numbers of arguments ---
> otherwise the problem will come right back.

Well I think this can be a pretty soft policy. The thing is that for
string_agg it's a pretty weak argument for the one-argument form
anyways so there's not much loss in losing the 1-argument form. In
other cases the extra arguments might be for very obscure cases or
there may be lots of precedent for the variadic form and users might
expect to have it. In which case we could decide the cost/benefit
calculation comes down the other way.

--
greg

Greg Stark <gsstark(at)mit(dot)edu> writes:
> On Wed, Aug 4, 2010 at 11:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> What we are doing here, IMO, is not just changing string_agg() but
>> instituting a project policy that we are not going to offer built-in
>> aggregates with the same names and different numbers of arguments ---
>> otherwise the problem will come right back.

> Well I think this can be a pretty soft policy.

Sure, we could break it given good cause. What I intend with the
opr_sanity test is just that people won't be able to put something
in without being reminded of this consideration.

regards, tom lane

2010/8/4 Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)gov>:
> Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>> On Wed, Aug 4, 2010 at 11:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.
>>

The same problem can be with custom aggregates :( so this syntax isn't
too robust. We can support Oracle's syntax in future releases, where
syntax divide aggregate call and ORDER BY clause.

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

It is just removing some from one perspective problematic code. This
doesn't add any feature - so it cannot be a precedents.

we can look on this situation from two views:

a) it is good, because we can document this feature/behave - without
one param aggregates people will repeat same situation with custom
aggregates- and this will not be documented.

b) it is bad, because lot of users will be confused.

I prefer @a

Regards

Pavel

>
> -Kevin
>
> --
> Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs
>

On Thu, Aug 5, 2010 at 5:18 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> The same problem can be with custom aggregates :( so this syntax isn't
> too robust. We can support Oracle's syntax in future releases, where
> syntax divide aggregate call and ORDER BY clause.
>

What syntax is that?

--
greg

On 5 August 2010 10:29, Greg Stark <gsstark(at)mit(dot)edu> wrote:
> On Thu, Aug 5, 2010 at 5:18 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> The same problem can be with custom aggregates :( so this syntax isn't
>> too robust. We can support Oracle's syntax in future releases, where
>> syntax divide aggregate call and ORDER BY clause.
>>
>
> What syntax is that?
>
> --
> greg
>

An example I've found is:

SELECT deptno, LISTAGG(ename, ',') WITHIN GROUP (ORDER BY ename) AS employees
FROM emp
GROUP BY deptno;

--
Thom Brown
Registered Linux user: #516935

Thom Brown <thom(at)linux(dot)com> writes:
> On 5 August 2010 10:29, Greg Stark <gsstark(at)mit(dot)edu> wrote:
>> On Thu, Aug 5, 2010 at 5:18 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> The same problem can be with custom aggregates :( so this syntax isn't
>>> too robust. We can support Oracle's syntax in future releases, where
>>> syntax divide aggregate call and ORDER BY clause.
>>
>> What syntax is that?

> An example I've found is:
> SELECT deptno, LISTAGG(ename, ',') WITHIN GROUP (ORDER BY ename) AS employees
> FROM emp
> GROUP BY deptno;

That wouldn't help this problem in the least. The problem is that
novices unfamiliar with the SQL-standard aggregrate ORDER BY syntax
may try to put the ORDER BY in the wrong place. Offering a different
syntax won't stop them from doing that. The only way it might stop
would be if we documented *only* the Oracle syntax and not the
spec-compliant syntax. Which ain't gonna happen.

[ does a bit more research ... ] Actually, the syntax Thom mentions
is not Oracle-specific; it's in SQL:2008, and AFAICT it means something
different from an aggregate ORDER BY anyway. Maybe Pavel had something
else in mind. But my point is still that offering a different syntax
doesn't fix the problem unless we eliminate the mistake-prone syntax;
which we can't because it's in the spec.

regards, tom lane

2010/8/5 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Thom Brown <thom(at)linux(dot)com> writes:
>> On 5 August 2010 10:29, Greg Stark <gsstark(at)mit(dot)edu> wrote:
>>> On Thu, Aug 5, 2010 at 5:18 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>>> The same problem can be with custom aggregates :( so this syntax isn't
>>>> too robust. We can support Oracle's syntax in future releases, where
>>>> syntax divide aggregate call and ORDER BY clause.
>>>
>>> What syntax is that?
>
>> An example I've found is:
>> SELECT deptno, LISTAGG(ename, ',') WITHIN GROUP (ORDER BY ename) AS employees
>> FROM   emp
>> GROUP BY deptno;
>
> That wouldn't help this problem in the least.  The problem is that
> novices unfamiliar with the SQL-standard aggregrate ORDER BY syntax
> may try to put the ORDER BY in the wrong place.  Offering a different
> syntax won't stop them from doing that.  The only way it might stop
> would be if we documented *only* the Oracle syntax and not the
> spec-compliant syntax.  Which ain't gonna happen.
>
> [ does a bit more research ... ]  Actually, the syntax Thom mentions
> is not Oracle-specific; it's in SQL:2008, and AFAICT it means something
> different from an aggregate ORDER BY anyway.  Maybe Pavel had something
> else in mind.  But my point is still that offering a different syntax
> doesn't fix the problem unless we eliminate the mistake-prone syntax;
> which we can't because it's in the spec.
>

I though this syntax - and what I know Oracle use it for explicit
order and I found lot of sources on net, where is syntax of aggregates
like

name(parameters) [within group ( order by ... ) ]

but my knowledge of this subject is minimal, based on Oracle doc, when
I worked on string_agg function.

I agree, so different syntax doesn't remove a risks, but can decrease
some risks. SQL has lot of a possible dangerous syntaxes and everybody
can selects the most robust syntax.

But this issue can be solved a better documentation.

Regards

Pavel

>                        regards, tom lane
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>>>> The same problem can be with custom aggregates :( so this syntax isn't
>>>> too robust.

BTW, I'm really not worried about that case. By the time someone is
advanced enough to have written their own multi-argument aggregate
definitions, they'll have absorbed the idea that the ORDER BY goes at
the end. What we need to accomplish here is just to not set traps at
the feet of novices using the feature for the first time. Which is
why I think it's sufficient to have a policy of not having built-in
aggregates that conflict in this way; I'm not proposing that we restrict
or discourage custom aggregates with optional arguments.

regards, tom lane

2010/8/5 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>>>>> The same problem can be with custom aggregates :( so this syntax isn't
>>>>> too robust.
>
> BTW, I'm really not worried about that case.  By the time someone is
> advanced enough to have written their own multi-argument aggregate
> definitions, they'll have absorbed the idea that the ORDER BY goes at
> the end.  What we need to accomplish here is just to not set traps at
> the feet of novices using the feature for the first time.  Which is
> why I think it's sufficient to have a policy of not having built-in
> aggregates that conflict in this way; I'm not proposing that we restrict
> or discourage custom aggregates with optional arguments.
>

+1

but still when we remove one parametric string_agg, then this issue
will not be documented.

Pavel

>                        regards, tom lane
>

On 5 August 2010 16:31, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2010/8/5 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>>>>>> The same problem can be with custom aggregates :( so this syntax isn't
>>>>>> too robust.
>>
>> BTW, I'm really not worried about that case.  By the time someone is
>> advanced enough to have written their own multi-argument aggregate
>> definitions, they'll have absorbed the idea that the ORDER BY goes at
>> the end.  What we need to accomplish here is just to not set traps at
>> the feet of novices using the feature for the first time.  Which is
>> why I think it's sufficient to have a policy of not having built-in
>> aggregates that conflict in this way; I'm not proposing that we restrict
>> or discourage custom aggregates with optional arguments.
>>
>
> +1
>
> but still when we remove one parametric string_agg, then this issue
> will not be documented.
>
> Pavel
>

I think the problem is people like me not reading this important
information:http://www.postgresql.org/docs/9.0/static/sql-expressions.html#SYNTAX-AGGREGATES

It's clear as day upon reading that. It's a case of one page reading:

string_agg(expression [, delimiter ] )

and another reading:

aggregate_name (expression [ , ... ] [ order_by_clause ] )

and you effectively end up with:

string_agg(expression [, delimiter ] [ order_by_clause ] )

--
Thom Brown
Registered Linux user: #516935

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> but still when we remove one parametric string_agg, then this issue
> will not be documented.

How so? This paragraph will still be there:

<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; -- incorrect
</programlisting>
The latter is syntactically valid, but it represents a call of a
single-argument aggregate function with two <literal>ORDER BY</> keys
(the second one being rather useless since it's a constant).
</para>

regards, tom lane

2010/8/5 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> but still when we remove one parametric string_agg, then this issue
>> will not be documented.
>
> How so?  This paragraph will still be there:
>
>   <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;  -- incorrect
> </programlisting>
>    The latter is syntactically valid, but it represents a call of a
>    single-argument aggregate function with two <literal>ORDER BY</> keys
>    (the second one being rather useless since it's a constant).
>   </para>
>
>
>                        regards, tom lane
>

ok

Regards

Pavel Stehule

I wrote:
> Well, I forgot that an aggregate involves more than one catalog row ;-).
> So it's a bit bigger patch than that, but still pretty small and safe.
> See attached.

Applied to HEAD and 9.0. The mistaken case will now yield this:

regression=# select string_agg(f1 order by f1, ',') from text_tbl;
ERROR: function string_agg(text) does not exist
LINE 1: select string_agg(f1 order by f1, ',') from text_tbl;
^
HINT: No function matches the given name and argument types. You might need to add explicit type casts.

It's not perfect (I don't think it's practical to get the HINT to
read "Put the ORDER BY at the end" ;-)) but at least it should
get people pointed in the right direction when they do this.

regards, tom lane

On ons, 2010-08-04 at 18:19 -0400, Tom Lane wrote:
>
> This policy also implies that we are never going to allow default
> arguments for aggregates, or at least never have any built-in ones
> that use such a feature.
>
> By my count the following people had offered an opinion on making
> this change:
> for: tgl, josh, badalex, mmoncure
> against: rhaas, thom
> Anybody else want to vote, or change their vote after seeing the
> patch?

I vote against this patch. There are plenty of other places that SQL is
confusing, and this move seems excessive to me, and I find the
functionality that is proposed for removal quite useful.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> I vote against this patch. There are plenty of other places that SQL is
> confusing, and this move seems excessive to me, and I find the
> functionality that is proposed for removal quite useful.

Huh? The functionality proposed for removal is only that of omitting an
explicit delimiter argument for string_agg(). Since the default value
(an empty string) doesn't seem to be the right thing all that often
anyway, I'm not following why you think this is a significant downgrade.

regards, tom lane

On Aug 5, 2010, at 11:25 AM, Tom Lane wrote:

> Applied to HEAD and 9.0. The mistaken case will now yield this:
>
> regression=# select string_agg(f1 order by f1, ',') from text_tbl;
> ERROR: function string_agg(text) does not exist
> LINE 1: select string_agg(f1 order by f1, ',') from text_tbl;
> ^

I'm confused: that looks like the two-argument form to me. Have I missed something?

> HINT: No function matches the given name and argument types. You might need to add explicit type casts.
>
> It's not perfect (I don't think it's practical to get the HINT to
> read "Put the ORDER BY at the end" ;-)) but at least it should
> get people pointed in the right direction when they do this.

It confuses the shit out of me. It says "string_agg(text)" doesn't exist when that clearly is not the name of the function you've called.

Best,

David

On 5 August 2010 19:39, David E. Wheeler <david(at)kineticode(dot)com> wrote:
> On Aug 5, 2010, at 11:25 AM, Tom Lane wrote:
>
>> Applied to HEAD and 9.0.  The mistaken case will now yield this:
>>
>> regression=# select string_agg(f1 order by f1, ',') from text_tbl;
>> ERROR:  function string_agg(text) does not exist
>> LINE 1: select string_agg(f1 order by f1, ',') from text_tbl;
>>               ^
>
> I'm confused: that looks like the two-argument form to me. Have I missed something?
>
>> HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
>>
>> It's not perfect (I don't think it's practical to get the HINT to
>> read "Put the ORDER BY at the end" ;-)) but at least it should
>> get people pointed in the right direction when they do this.
>
> It confuses the shit out of me. It says "string_agg(text)" doesn't exist when that clearly is not the name of the function you've called.
>

What function name do you believe was called?

--
Thom Brown
Registered Linux user: #516935

On Thu, Aug 5, 2010 at 12:25, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> regression=# select string_agg(f1 order by f1, ',') from text_tbl;
> ERROR:  function string_agg(text) does not exist
> LINE 1: select string_agg(f1 order by f1, ',') from text_tbl;
>               ^
> HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
>
> It's not perfect (I don't think it's practical to get the HINT to
> read "Put the ORDER BY at the end" ;-)) but at least it should
> get people pointed in the right direction when they do this.

Not to mention I think most of the confusion came from using the 1
argument version first (with an order by) and then jumping straight to
the 2 arg version.

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On Aug 5, 2010, at 11:25 AM, Tom Lane wrote:
>> Applied to HEAD and 9.0. The mistaken case will now yield this:
>> regression=# select string_agg(f1 order by f1, ',') from text_tbl;
>> ERROR: function string_agg(text) does not exist

> I'm confused: that looks like the two-argument form to me. Have I missed something?

Yeah, the whole point of the thread: that's not a call of a two-argument
aggregate. It's a call of a one-argument aggregate, using a two-column
sort key to order the aggregate input rows.

> It confuses the shit out of me. It says "string_agg(text)" doesn't exist when that clearly is not the name of the function you've called.

Well, maybe we need to expend some more sweat on the error message then.
But this patch was still a prerequisite thing, because without it there
is no error that we can complain about.

regards, tom lane

On Aug 5, 2010, at 11:42 AM, Thom Brown wrote:

>>> LINE 1: select string_agg(f1 order by f1, ',') from text_tbl;
>>> ^
>>
>> I'm confused: that looks like the two-argument form to me. Have I missed something?
>>
>>> HINT: No function matches the given name and argument types. You might need to add explicit type casts.
>>>
>>> It's not perfect (I don't think it's practical to get the HINT to
>>> read "Put the ORDER BY at the end" ;-)) but at least it should
>>> get people pointed in the right direction when they do this.
>>
>> It confuses the shit out of me. It says "string_agg(text)" doesn't exist when that clearly is not the name of the function you've called.
>>
>
> What function name do you believe was called?

The message says:

string_agg(f1 order by f1, ',')

That looks like string_agg(text, text) or string_agg(anyelement, text).

Best,

David

On Aug 5, 2010, at 11:45 AM, Tom Lane wrote:

>> I'm confused: that looks like the two-argument form to me. Have I missed something?
>
> Yeah, the whole point of the thread: that's not a call of a two-argument
> aggregate. It's a call of a one-argument aggregate, using a two-column
> sort key to order the aggregate input rows.

OH!. Wow, weird. I never noticed that.

>> It confuses the shit out of me. It says "string_agg(text)" doesn't exist when that clearly is not the name of the function you've called.
>
> Well, maybe we need to expend some more sweat on the error message then.
> But this patch was still a prerequisite thing, because without it there
> is no error that we can complain about.

Yeah, understood.

Best,

David

> Well, maybe we need to expend some more sweat on the error message then.
> But this patch was still a prerequisite thing, because without it there
> is no error that we can complain about.

Yes, I'd say an addition to the HINT is in order *assuming* at that
stage we can tell if the user passed an ORDER BY or not.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> Well, maybe we need to expend some more sweat on the error message then.
>> But this patch was still a prerequisite thing, because without it there
>> is no error that we can complain about.

> Yes, I'd say an addition to the HINT is in order *assuming* at that
> stage we can tell if the user passed an ORDER BY or not.

I was just looking at this, and realized I was mistaken earlier: the
error is issued in ParseFuncOrColumn, which already is passed the
agg_order list, so actually it's completely trivial to tell whether
a variant error message is appropriate. I suggest that we key it off
there being not just an ORDER BY, but an ORDER BY with more than one
element; if there's only one then this cannot be the source of
confusion.

Next question: exactly how should the variant HINT be phrased?
I'm inclined to drop the bit about explicit casts and make it read
something like

HINT: No aggregate function matches the given name and argument
types. Perhaps you misplaced ORDER BY; ORDER BY must appear after all
regular arguments of the aggregate.

regards, tom lane

On Thu, Aug 5, 2010 at 3:16 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>>> Well, maybe we need to expend some more sweat on the error message then.
>>> But this patch was still a prerequisite thing, because without it there
>>> is no error that we can complain about.
>
>> Yes, I'd say an addition to the HINT is in order *assuming* at that
>> stage we can tell if the user passed an ORDER BY or not.
>
> I was just looking at this, and realized I was mistaken earlier: the
> error is issued in ParseFuncOrColumn, which already is passed the
> agg_order list, so actually it's completely trivial to tell whether
> a variant error message is appropriate.  I suggest that we key it off
> there being not just an ORDER BY, but an ORDER BY with more than one
> element; if there's only one then this cannot be the source of
> confusion.
>
> Next question: exactly how should the variant HINT be phrased?
> I'm inclined to drop the bit about explicit casts and make it read
> something like
>
> HINT: No aggregate function matches the given name and argument
> types. Perhaps you misplaced ORDER BY; ORDER BY must appear after all
> regular arguments of the aggregate.

Could we arrange to emit this error message only when there is an
aggregate with the same name but different arguments?

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

On Aug 5, 2010, at 12:16 PM, Tom Lane wrote:

> HINT: No aggregate function matches the given name and argument
> types. Perhaps you misplaced ORDER BY; ORDER BY must appear after all
> regular arguments of the aggregate.

+1

David

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Aug 5, 2010 at 3:16 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Next question: exactly how should the variant HINT be phrased?
>> I'm inclined to drop the bit about explicit casts and make it read
>> something like
>>
>> HINT: No aggregate function matches the given name and argument
>> types. Perhaps you misplaced ORDER BY; ORDER BY must appear after all
>> regular arguments of the aggregate.

> Could we arrange to emit this error message only when there is an
> aggregate with the same name but different arguments?

That'd move it into the category of needing significant restructuring,
I'm afraid. At the moment it's not apparent that it's worth it.
We're already able to limit the use of the variant hint to a pretty
darn narrow set of cases, and it is only a hint after all.

regards, tom lane

On 8/5/10 12:18 PM, Robert Haas wrote:
> Could we arrange to emit this error message only when there is an
> aggregate with the same name but different arguments?

Personally, I don't see this as really necessary. Just mentioning ORDER
BY in the hint will be enough to give people the right place to look.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> On 8/5/10 12:18 PM, Robert Haas wrote:
>> Could we arrange to emit this error message only when there is an
>> aggregate with the same name but different arguments?

> Personally, I don't see this as really necessary. Just mentioning ORDER
> BY in the hint will be enough to give people the right place to look.

I suppose Robert is more concerned about the possibility that we emit
the ORDER BY hint when that isn't really the source of the problem.
But after all, the reason it's a hint and not the primary error message
is that it's not certain to be helpful.

regards, tom lane

On tor, 2010-08-05 at 14:38 -0400, Tom Lane wrote:
> Huh? The functionality proposed for removal is only that of omitting
> an explicit delimiter argument for string_agg(). Since the default
> value (an empty string) doesn't seem to be the right thing all that
> often anyway, I'm not following why you think this is a significant
> downgrade.

I just think it's useful to have the one-argument version. I understand
the functionality is available in other ways.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On tor, 2010-08-05 at 14:38 -0400, Tom Lane wrote:
>> Huh? The functionality proposed for removal is only that of omitting
>> an explicit delimiter argument for string_agg(). Since the default
>> value (an empty string) doesn't seem to be the right thing all that
>> often anyway, I'm not following why you think this is a significant
>> downgrade.

> I just think it's useful to have the one-argument version. I understand
> the functionality is available in other ways.

Well, other things being equal I'd have preferred to keep the
one-argument version too. But this thread has made it even clearer than
before that we will get continuing bug reports if we leave the behavior
alone. I don't think the ability to leave off the delimiter value is
worth the amount of confusion it'll cause.

regards, tom lane