Re: SearchSysCacheTuple(Copy) - Mailing list pgsql-hackers

I said:
> This class of bugs has been there since the beginning of Postgres,
> so I do not feel that we need to panic about it.  Let's take the
> time to design and implement a proper solution, rather than expending
> effort on a stopgap solution that'll have to be undone later.

I've reviewed the earlier threads on SearchSysCache and done some more
thinking.  Here is a concrete proposal for doing it right:

1. Add a reference-count field and an already-dead boolean to each  syscache entry.  (The already-dead flag indicates
thatwe received  an SI inval message for this tuple, but we couldn't delete it yet  because it has refcount > 0.  A
tuplewith this flag set will never  be returned by a new SearchSysCache call, and it will be deleted  from the cache as
soonas its refcount goes to zero.)
 

2. SearchSysCache() will increment the reference count of the tuple  it returns.

3. A new entry point ReleaseSysCache() will be provided to decrement  the reference count of a syscache entry.  Thus,
thestandard calling  sequence becomes
 
tuple = SearchSysCacheTuple(...);if (HeapTupleIsValid(tuple)){    ... use tuple ...    ReleaseSysCache(tuple);}

4. SearchSysCacheTupleCopy() goes away, since we may as well use  SearchSysCacheTuple() and ReleaseSysCache() instead
of SearchSysCacheTupleCopy() and heap_freetuple().
 

5. Since SearchSysCache() is called from exactly one place, namely  SearchSysCacheTuple(), I am inclined to rename
SearchSysCache() to SearchCatCache() and then give the name SearchSysCache() to  the more widely used routine.  So
SearchSysCache()and  ReleaseSysCache() would really be the widely used pair of routines.
 

6. When a shared-cache-inval message arrives, the syscache code  behaves as follows for each affected cache entry:  A.
Ifrefcount is zero, just delete the entry.  B. Otherwise, set the "already-dead" flag, so that future cache
searcheswill not return this tuple and it will be released     once its refcount reaches zero.
 

7. At end of transaction (whether normal or abort), scan the syscaches  to reset refcounts to zero and delete any
marked-deadentries.  We  should not consider it a software error to leave syscache entries  still locked at end of
transaction. (The parser, in particular,  would need a lot of work to avoid doing so, and I don't see much  value in
expendingsuch work.)
 

Note that this proposal does not include any attempt to detect whether
a cache inval message means that the tuple has actually been changed
(as opposed to just relocated, for example).  It seems too expensive
to go out and re-read such tuples, and I'm not sure that it's safe to
try to read new cache entries during cache inval anyway.  Besides,
most callers that are using a cache entry are happy to continue to use
the copy that was valid when they got it --- they don't care if a
subsequent transaction commit changes the tuple.

Callers that want to be certain they have a completely-up-to-date copy
should acquire a suitable lock on the associated system relation before
calling SearchSysCache().  In practice, the only callers that really need
this are places that are going to update or delete the tuple, and so they
need to acquire a write lock on the system relation anyway.  The coding
rule is then just "lock the relation before finding the tuple to update,
not after".  We have that rule in place already.

Comments?  I think I might have time to do this before Vadim finishes
with WAL ;-)
        regards, tom lane