The fact that the collations patch put fn_collation into FmgrInfo,
rather than FunctionCallInfo, has been bothering me for awhile.  The
collation is really a kind of argument, not a property of the function,
so FmgrInfo is logically the wrong place for it.  But I'd not found a
concrete reason not to do it that way.  Now I think I have.  Bug #5970
points out that record_cmp() needs to set up collations for the
comparison functions it calls.  Since record_cmp relies on FmgrInfo
structs that belong to the typcache, this is problematic.  I see three

1.  Scribble on fn_collation of the FmgrInfo, even though it's in a
cache entry that may be used by other calls.  This is only safe if
you assume that record_cmp (and array_cmp, which is already doing this)
need not be re-entrant, ie the cache entry won't be used for another
purpose before we're done with the comparison.  Considering that the
comparison function can be user-defined code, I don't find that
assumption safe in the slightest.

2.  Copy the FmgrInfo struct to local storage in record_cmp (ick).
Since these FmgrInfo structs advertise that they belong to
CacheMemoryContext, that doesn't seem very safe either.  A function
could allocate fn_extra workspace in CacheMemoryContext, and then do it
over again on the next call, lather rinse repeat.  Maybe we could fix
that by copying the fn_extra pointer *back* to the typcache afterwards,
but double ick.  (And that doesn't seem very safe if the typcache entry
could get used re-entrantly, anyway.)

3.  Don't store fn_collation in FmgrInfo.

A short look around the code suggests that #3 may not be inordinately
painful.  We'd need to add a collation field to ScanKey to make up for
the lack of one in the contained FmgrInfo, but that would make the code
cleaner not dirtier.  I can see a couple of places where the index AMs
assume that the index's collation is available from index_getprocinfo,
but it doesn't look too terribly hard to get them to consult
index->rd_indcollation[] instead.

So, unless there's a really good reason why fn_collation should be in
FmgrInfo and not FunctionCallInfo, I'm going to see about moving it.

                        regards, tom lane

Sent via pgsql-hackers mailing list (
To make changes to your subscription:

Reply via email to