Re: [HACKERS] fn_collation in FmgrInfo considered harmful
I wrote: 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. It looks like the single largest PITA involved in this change is that the FunctionCallN/OidFunctionCallN/DirectFunctionCallN families of functions really ought to take a collation argument, and there are approximately 540 existing calls of those functions in the source tree. Of those calls, a pretty substantial majority don't really need collation info, because they are calling functions that are known not to care about collations. So while I could go around and add an InvalidOid argument to each one, it seems like an invasive change for rather small benefit. What I'm thinking about doing instead is establishing these conventions: 1. The existing names with a C appended (eg, OidFunctionCall2C) will take a collation argument (in particular, this replaces the existing DirectFunctionCall1WithCollation and DirectFunctionCall2WithCollation, which seem a bit verbosely named for my tastes). 2. The actual functions in fmgr.c will just be the C versions. We'll preserve source-level compatibility by #define'ing the old names as macros that expand to call the C functions with InvalidOid for the collation. This will avoid needing source-code changes except in the places where collations actually have to be passed. I don't think this is quite what we would have done if starting in a green field, but I doubt it's worth the hassle to convert all the existing calls to add an argument. Objections, better ideas? 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
Re: [HACKERS] fn_collation in FmgrInfo considered harmful
On Tuesday, April 12, 2011 08:09:53 PM Tom Lane wrote: 1. The existing names with a C appended (eg, OidFunctionCall2C) will take a collation argument (in particular, this replaces the existing DirectFunctionCall1WithCollation and DirectFunctionCall2WithCollation, which seem a bit verbosely named for my tastes). The first thing I though when I saw OidFunctionCall2C was Function Call with C collation or such and that you wanted to rename all the existing calls to that... Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fn_collation in FmgrInfo considered harmful
Andres Freund and...@anarazel.de writes: On Tuesday, April 12, 2011 08:09:53 PM Tom Lane wrote: 1. The existing names with a C appended (eg, OidFunctionCall2C) will take a collation argument (in particular, this replaces the existing DirectFunctionCall1WithCollation and DirectFunctionCall2WithCollation, which seem a bit verbosely named for my tastes). The first thing I though when I saw OidFunctionCall2C was Function Call with C collation or such and that you wanted to rename all the existing calls to that... Hm, well, you got a better idea? I definitely want it *short*, because these are going to be in a lot of places. 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
Re: [HACKERS] fn_collation in FmgrInfo considered harmful
On Tuesday, April 12, 2011 09:00:40 PM Tom Lane wrote: Andres Freund and...@anarazel.de writes: On Tuesday, April 12, 2011 08:09:53 PM Tom Lane wrote: 1. The existing names with a C appended (eg, OidFunctionCall2C) will take a collation argument (in particular, this replaces the existing DirectFunctionCall1WithCollation and DirectFunctionCall2WithCollation, which seem a bit verbosely named for my tastes). The first thing I though when I saw OidFunctionCall2C was Function Call with C collation or such and that you wanted to rename all the existing calls to that... Hm, well, you got a better idea? I definitely want it *short*, because these are going to be in a lot of places. Not really. Maybe DirectFunctionCall1Coll or even DirectFCall1Coll... Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fn_collation in FmgrInfo considered harmful
Andres Freund and...@anarazel.de writes: On Tuesday, April 12, 2011 09:00:40 PM Tom Lane wrote: Hm, well, you got a better idea? I definitely want it *short*, because these are going to be in a lot of places. Not really. Maybe DirectFunctionCall1Coll or even DirectFCall1Coll... xxxFunctionCallNColl would probably work. 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
[HACKERS] fn_collation in FmgrInfo considered harmful
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
