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

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> On Wed, Mar 09, 2011 at 04:49:28PM -0500, Tom Lane wrote:
>> 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.

Hmm. That suggests a third solution: revert the addition of *all* the
collid fields except the ones that represent collation-to-apply-during-
function-execution. (So they'd still be there in FuncExpr/OpExpr, but
not most other places.) Then we'd have to dig down more deeply in the
expression tree during select_common_collation, but we'd save space
and avoid confusion over the meaning of the fields.

I suspect this is probably not a good idea because of the added cost in
select_common_collation: aside from probably needing more syscache
lookups, there's a potential for worse-than-linear cost behavior if we
have to repeatedly dig through a deep expression tree to find out
collations. We had a similar case in the past [ checks archives ... see
http://archives.postgresql.org/pgsql-performance/2005-06/msg00075.php
http://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=ba4200246
] so I'm hesitant to go down that road again. Still, I'll throw it out
for comment.

regards, tom lane