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.
The trouble is that these usages are only compatible if both the
arguments and result of the function are of collatable types. In
particular, the change I made was causing CREATE VIEW to fail on
examples like this:
regression=# create view vv as select 'z'::text < 'y'::text as b;
ERROR: collations are not supported by type boolean
because the OpExpr node must have nonzero collid to do a textual
comparison, but then exprCollation() claims that the result of the
expression has that collation too, which it does not because it's
only bool.
Aside from confusing code that tries to impute a collation to the
result of an expression, this will confuse the collation assignment code
itself: select_common_collation will mistakenly believe that
non-collatable input arguments should affect its results. I'm not sure
how you managed to avoid such failures in the committed patch (if indeed
you did avoid them and they're not just lurking in un-regression-tested
cases); but in any case it seems far too fragile to keep it this way.
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.
Of course the syscache lookup implied by type_is_collatable will mean
that exprCollation is orders of magnitude slower than it is now. So
this is a pretty straightforward space vs speed tradeoff. I'm inclined
to think that choice #1 is the lesser evil, because I'm afraid that this
patch has already added an undesirable number of new cache lookups to
the basic expression parsing paths.
Thoughts?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
