Fix a problem introduced by my patch of 2010-01-12 that revised the way

relcache reload works.  In the patched code, a relcache entry in process of
being rebuilt doesn't get unhooked from the relcache hash table; which means
that if a cache flush occurs due to sinval queue overrun while we're
rebuilding it, the entry could get blown away by RelationCacheInvalidate,
resulting in crash or misbehavior.  Fix by ensuring that an entry being
rebuilt has positive refcount, so it won't be seen as a target for removal
if a cache flush occurs.  (This will mean that the entry gets rebuilt twice
in such a scenario, but that's okay.)  It appears that the problem can only
arise within a transaction that has previously reassigned the relfilenode of
a pre-existing table, via TRUNCATE or a similar operation.  Per bug #5412
from Rusty Conover.

Back-patch to 8.2, same as the patch that introduced the problem.
I think that the failure can't actually occur in 8.2, since it lacks the
rd_newRelfilenodeSubid optimization, but let's make it work like the later
branches anyway.

Patch by Heikki, slightly editorialized on by me.

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 4ed05623573c439e69f38c8718060799c2d0e87b..ee6eef1a022ccb3a71dee399cc66d0bb2f2ad9e4 100644 (file)
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/cache/relcache.c,v 1.266.2.8 2010/01/13 23:07:22 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/cache/relcache.c,v 1.266.2.9 2010/04/14 21:31:27 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1757,18 +1757,33 @@ RelationDestroyRelation(Relation relation)
  *
  *      Physically blow away a relation cache entry, or reset it and rebuild
  *      it from scratch (that is, from catalog entries).  The latter path is
- *      usually used when we are notified of a change to an open relation
- *      (one with refcount > 0).  However, this routine just does whichever
- *      it's told to do; callers must determine which they want.
- *
- *      NB: when rebuilding, we'd better hold some lock on the relation.
- *      In current usages this is presumed true because it has refcnt > 0.
+ *      used when we are notified of a change to an open relation (one with
+ *      refcount > 0).
+ *
+ *      NB: when rebuilding, we'd better hold some lock on the relation,
+ *      else the catalog data we need to read could be changing under us.
+ *      Also, a rel to be rebuilt had better have refcnt > 0.  This is because
+ *      an sinval reset could happen while we're accessing the catalogs, and
+ *      the rel would get blown away underneath us by RelationCacheInvalidate
+ *      if it has zero refcnt.
+ *
+ *      The "rebuild" parameter is redundant in current usage because it has
+ *      to match the relation's refcnt status, but we keep it as a crosscheck
+ *      that we're doing what the caller expects.
  */
 static void
 RelationClearRelation(Relation relation, bool rebuild)
 {
        Oid                     old_reltype = relation->rd_rel->reltype;
 
+       /*
+        * As per notes above, a rel to be rebuilt MUST have refcnt > 0; while
+        * of course it would be a bad idea to blow away one with nonzero refcnt.
+        */
+       Assert(rebuild ?
+                  !RelationHasReferenceCountZero(relation) :
+                  RelationHasReferenceCountZero(relation));
+
        /*
         * Make sure smgr and lower levels close the relation's files, if they
         * weren't closed already.  If the relation is not getting deleted, the
@@ -1962,8 +1977,6 @@ RelationClearRelation(Relation relation, bool rebuild)
 static void
 RelationFlushRelation(Relation relation)
 {
-       bool            rebuild;
-
        if (relation->rd_createSubid != InvalidSubTransactionId ||
                relation->rd_newRelfilenodeSubid != InvalidSubTransactionId)
        {
@@ -1971,18 +1984,24 @@ RelationFlushRelation(Relation relation)
                 * New relcache entries are always rebuilt, not flushed; else we'd
                 * forget the "new" status of the relation, which is a useful
                 * optimization to have.  Ditto for the new-relfilenode status.
+                *
+                * The rel could have zero refcnt here, so temporarily increment
+                * the refcnt to ensure it's safe to rebuild it.  We can assume that
+                * the current transaction has some lock on the rel already.
                 */
-               rebuild = true;
+               RelationIncrementReferenceCount(relation);
+               RelationClearRelation(relation, true);
+               RelationDecrementReferenceCount(relation);
        }
        else
        {
                /*
                 * Pre-existing rels can be dropped from the relcache if not open.
                 */
-               rebuild = !RelationHasReferenceCountZero(relation);
-       }
+               bool    rebuild = !RelationHasReferenceCountZero(relation);
 
-       RelationClearRelation(relation, rebuild);
+               RelationClearRelation(relation, rebuild);
+       }
 }
 
 /*
@@ -2287,7 +2306,6 @@ AtEOSubXact_RelationCache(bool isCommit, SubTransactionId mySubid,
                                relation->rd_createSubid = parentSubid;
                        else
                        {
-                               Assert(RelationHasReferenceCountZero(relation));
                                RelationClearRelation(relation, false);
                                continue;
                        }