I've been running the regression tests with the sinval-reset stress
testing program I posted yesterday:
http://archives.postgresql.org/pgsql-hackers/2006-01/msg00244.php
I've seen several crashes caused by using a tuple descriptor obtained
from lookup_rowtype_tupdesc() after the descriptor has been recycled
due to cache flushes.
On reflection I think that lookup_rowtype_tupdesc is simply misdesigned.
We can't have it handing back a pointer to a data structure of unspecified
lifetime. One possibility is to give it an API comparable to the
syscache lookup functions, ie you get a reference-counted pointer that
you have to explicitly release when done with it. This would also
presumably require extending the ResourceOwner mechanism to track these
reference counts.
A simpler alternative is just to make every caller copy the passed-back
tupdesc immediately if it's going to do more than the most trivial work
with it; or even put the copying step right into lookup_rowtype_tupdesc
to make sure no one can omit it. Here we'd be paying palloc and data
copying overhead to keep the code simple. However, it's arguable that
none of the uses of lookup_rowtype_tupdesc are performance-critical
enough to justify adding a lot of infrastructure to avoid this.
One big strike against the reference-count approach is that it'd be
difficult to back-patch such a solution into existing branches, since
it would amount to an incompatible API change. If there are any add-on
modules out there that use lookup_rowtype_tupdesc, they'd start failing
because of not releasing the pointer.
Any opinions what to do?
regards, tom lane
