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
choices:
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
