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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers