Thread: select_common_collation callers way too ready to throw error

db1=# create table t1 (f1 text collate "aa_DJ.utf8", f2 text collate "an_ES.utf8" );
CREATE TABLE
db1=# select f1 < f2 from t1;
ERROR:  collation mismatch between implicit collations "aa_DJ.utf8" and "an_ES.utf8"
LINE 1: select f1 < f2 from t1;                   ^
HINT:  You can override the collation by applying the COLLATE clause to one or both expressions.
db1=# select f1 || f2 from t1;
ERROR:  collation mismatch between implicit collations "aa_DJ.utf8" and "an_ES.utf8"
LINE 1: select f1 || f2 from t1;                    ^
HINT:  You can override the collation by applying the COLLATE clause to one or both expressions.

The first of these errors is OK, but surely the second is not, because ||
doesn't give a fig about collations.  I think instead of this:
/* XXX: If we knew which functions required collation information, * we could selectively set the last argument to true
here.*/funccollid = select_common_collation(pstate, fargs, false);
 

we need:
/* * If we knew which functions required collation information, * we could selectively set the last argument to false
here,* allowing the error to be reported at parse time not runtime. */funccollid = select_common_collation(pstate,
fargs,true);
 

Now the downside of that is that a runtime failure won't give you an
parse error pointer to indicate which function is having trouble ...
but having an error pointer for an error that shouldn't be thrown in
the first place is small consolation.
        regards, tom lane

On ons, 2011-03-09 at 18:07 -0500, Tom Lane wrote:
> The first of these errors is OK, but surely the second is not, because
> ||
> doesn't give a fig about collations.  I think instead of this:
> 
>         /* XXX: If we knew which functions required collation
> information,
>          * we could selectively set the last argument to true here. */
>         funccollid = select_common_collation(pstate, fargs, false);
> 
> we need:
> 
>         /*
>          * If we knew which functions required collation information,
>          * we could selectively set the last argument to false here,
>          * allowing the error to be reported at parse time not
> runtime.
>          */
>         funccollid = select_common_collation(pstate, fargs, true);
> 
> Now the downside of that is that a runtime failure won't give you an
> parse error pointer to indicate which function is having trouble ...
> but having an error pointer for an error that shouldn't be thrown in
> the first place is small consolation.

Sounds reasonable.

Btw., the ultimate plan here was that functions or operators that would
care about collation would be marked as such in pg_proc.  That plan
basically just ran out of time, but if you think it'd be useful, maybe
we could reactivate it quickly.

Peter Eisentraut <peter_e@gmx.net> writes:
> On ons, 2011-03-09 at 18:07 -0500, Tom Lane wrote:
>> Now the downside of that is that a runtime failure won't give you an
>> parse error pointer to indicate which function is having trouble ...
>> but having an error pointer for an error that shouldn't be thrown in
>> the first place is small consolation.

> Sounds reasonable.

> Btw., the ultimate plan here was that functions or operators that would
> care about collation would be marked as such in pg_proc.  That plan
> basically just ran out of time, but if you think it'd be useful, maybe
> we could reactivate it quickly.

I think it's worth doing at some point, but not right now.  By the time
you get done adding support for the flag to CREATE FUNCTION, pg_dump,
etc, it's not a small task.
        regards, tom lane