Обсуждение: 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
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
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
Andres Freund <andres@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
On Tuesday, April 12, 2011 09:00:40 PM Tom Lane wrote: > Andres Freund <andres@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
Andres Freund <andres@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