Thread: Bug Fix: COLLATE with multiple ORDER BYs in aggregates

Bug Fix: COLLATE with multiple ORDER BYs in aggregates

From
David Fetter
Date:
Folks,

While testing the upcoming FILTER clause for aggregates, Erik Rijkers
uncovered a long-standing bug in $subject, namely that this case
wasn't handled.  Please find attached a patch by Andrew Gierth and
myself which fixes this issue and adds a regression test to ensure it
remains fixed.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.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

Attachment

Examples Re: Bug Fix: COLLATE with multiple ORDER BYs in aggregates

From
David Fetter
Date:
On Tue, Apr 23, 2013 at 09:57:27AM -0700, David Fetter wrote:
> Folks,
> 
> While testing the upcoming FILTER clause for aggregates, Erik Rijkers
> uncovered a long-standing bug in $subject, namely that this case
> wasn't handled.  Please find attached a patch by Andrew Gierth and
> myself which fixes this issue and adds a regression test to ensure it
> remains fixed.

Please see below results when I run the regression test query on git
master, REL_9_2_STABLE, and REL9_1_STABLE, respectively:

$ psql postgres
psql (9.3devel)
Type "help" for help.

shackle@postgres:5493=# SELECT array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX") FROM (VALUES ('foo','bar')) v(a,b);
ERROR:  collation mismatch between explicit collations "C" and "POSIX"
LINE 1: SELECT array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX") F...
^

$ psql postgres
psql (9.2.4)
Type "help" for help.

shackle@postgres:5492=# SELECT array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX") FROM (VALUES ('foo','bar')) v(a,b);
ERROR:  collation mismatch between explicit collations "C" and "POSIX"
LINE 1: SELECT array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX") F...
^

$ psql postgres
psql (9.1.9)
Type "help" for help.

shackle@postgres:5491=# SELECT array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX") FROM (VALUES ('foo','bar')) v(a,b);
ERROR:  collation mismatch between explicit collations "C" and "POSIX"
LINE 1: SELECT array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX") F...
^
Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.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



Re: Bug Fix: COLLATE with multiple ORDER BYs in aggregates

From
Tom Lane
Date:
David Fetter <david@fetter.org> writes:
> While testing the upcoming FILTER clause for aggregates, Erik Rijkers
> uncovered a long-standing bug in $subject, namely that this case
> wasn't handled.  Please find attached a patch by Andrew Gierth and
> myself which fixes this issue and adds a regression test to ensure it
> remains fixed.

I don't find this patch to be a good idea.

The argument for it seems to be that

array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX")

should not throw an error, but why not?  And what does that have to do
with whacking around the code for CASE?
        regards, tom lane



Re: Bug Fix: COLLATE with multiple ORDER BYs in aggregates

From
Andres Freund
Date:
On 2013-04-25 13:42:32 -0400, Tom Lane wrote:
> David Fetter <david@fetter.org> writes:
> > While testing the upcoming FILTER clause for aggregates, Erik Rijkers
> > uncovered a long-standing bug in $subject, namely that this case
> > wasn't handled.  Please find attached a patch by Andrew Gierth and
> > myself which fixes this issue and adds a regression test to ensure it
> > remains fixed.
> 
> I don't find this patch to be a good idea.
> 
> The argument for it seems to be that
> 
> array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX")
> 
> should not throw an error, but why not? 

Uh. Why should it? SELECT foo COLLATE "C" FROM ... ORDER BY bar COLLATE
"POSIX" doesn't throw one either?

> And what does that have to do with whacking around the code for CASE?

I guess that's to avoid to repeat that already triplicated block of code
once more. The goal seems to make sense to me, although I am not 100%
that thats the nicest solution to get of the repetition.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: Bug Fix: COLLATE with multiple ORDER BYs in aggregates

From
Tom Lane
Date:
Andres Freund <andres@2ndquadrant.com> writes:
> On 2013-04-25 13:42:32 -0400, Tom Lane wrote:
>> The argument for it seems to be that
>> array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX")
>> should not throw an error, but why not? 

> Uh. Why should it? SELECT foo COLLATE "C" FROM ... ORDER BY bar COLLATE
> "POSIX" doesn't throw one either?

After thinking about it a bit more, this case *should* throw an error:
string_agg(a COLLATE "C", b COLLATE "POSIX")

but these should not:
array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX")
array_agg(a ORDER BY b COLLATE "C", c COLLATE "POSIX")

that is, the ORDER BY expression(s) ought to be considered independently
rather than as part of the agg's argument list.

It looks like the proposed patch gets this right, but the proposed
test cases really fail to illuminate the problem IMO.
        regards, tom lane



Re: Bug Fix: COLLATE with multiple ORDER BYs in aggregates

From
David Fetter
Date:
On Thu, Apr 25, 2013 at 06:04:10PM -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2013-04-25 13:42:32 -0400, Tom Lane wrote:
> >> The argument for it seems to be that
> >> array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX")
> >> should not throw an error, but why not? 
> 
> > Uh. Why should it? SELECT foo COLLATE "C" FROM ... ORDER BY bar COLLATE
> > "POSIX" doesn't throw one either?
> 
> After thinking about it a bit more, this case *should* throw an error:
> 
>     string_agg(a COLLATE "C", b COLLATE "POSIX")
> 
> but these should not:
> 
>     array_agg(a COLLATE "C" ORDER BY b COLLATE "POSIX")
> 
>     array_agg(a ORDER BY b COLLATE "C", c COLLATE "POSIX")
> 
> that is, the ORDER BY expression(s) ought to be considered independently
> rather than as part of the agg's argument list.
> 
> It looks like the proposed patch gets this right, but the proposed
> test cases really fail to illuminate the problem IMO.
> 
>             regards, tom lane

Am I understanding correctly that you want the code left alone and the
test case expanded as above?

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.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



Re: Bug Fix: COLLATE with multiple ORDER BYs in aggregates

From
Tom Lane
Date:
David Fetter <david@fetter.org> writes:
> While testing the upcoming FILTER clause for aggregates, Erik Rijkers
> uncovered a long-standing bug in $subject, namely that this case
> wasn't handled.  Please find attached a patch by Andrew Gierth and
> myself which fixes this issue and adds a regression test to ensure it
> remains fixed.

Applied with minor editorialization.
        regards, tom lane