Re: FuncExpr.collid/OpExpr.collid unworkably serving double duty

On Wed, Mar 09, 2011 at 04:49:28PM -0500, Tom Lane wrote:
> So I was moving some error checks around and all of a sudden the
> regression tests blew up on me, with lots of errors about how type X
> didn't support collations (which indeed it didn't). After some
> investigation I realized what should have been apparent much earlier:
> the collations patch is trying to use one field for two different
> purposes. In particular, collid in FuncExpr and related nodes is
> used in both of these ways:
>
> * as the collation to apply during execution of the function;
>
> * as the collation of the function's result.

Ouch, that is painful.

Looking back at my first attempt I see I made the same error, though I
had noted that it had an unusual failure mode, namely that:

func( a COLLATE x ) COLLATE y

would determine that "x" was the collation to use for func, not "y",
and that "y" may be ignored. A bit of a corner case, but someone was
bound to try it.

I think I avoided the particular failure mode you found, because the
GetCollation on the FuncExpr didn't return the collation calculated for
the node, but the the collation derived from the collations of any
arguments that had the same type and the return value. So operators
like = and < automatically got NONE because none of their arguments are
booleans.

> regression=# create view vv as select 'z'::text < 'y'::text as b;
> ERROR: collations are not supported by type boolean

I'm my original idea, any data type was collatable, since I considered
ASC and DESC, NULLS FIRST/LAST to be collations every datatype had.
Thus the above wasn't an error. As long as the collation was a
collation appropriate for booleans it worked.

> There are basically two things we could do about this:
>
> 1. Add two fields not one to nodes representing function/operator calls.
>
> 2. Change exprCollation() to do a type_is_collatable check on the
> node result type before believing that the collid field is relevant.

It might be worthwhile adding an extra field, but I think I didn't do
it because you only need the information exactly once, while descending
the parse tree in parse_expr. But for clarity the extra field is a
definite win.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patriotism is when love of your own people comes first; nationalism,
> when hate for people other than your own comes first.
> - Charles de Gaulle