fn_collation in FmgrInfo considered harmful

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers(at)postgreSQL(dot)org
Subject: fn_collation in FmgrInfo considered harmful
Date: 2011-04-11 19:44:12
Message-ID: 7742.1302551052@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jesper Krogh 2011-04-11 20:07:33 Re: Locking when concurrent updated of foreign references
Previous Message Pavel Stehule 2011-04-11 19:34:55 Re: Global variables in plpgsql