Forget about targeting catalog cache invalidations by tuple TID.

The TID isn't stable enough: we might queue an sinval event before a VACUUM
FULL, and then process it afterwards, when the target tuple no longer has
the same TID.  So we must invalidate entries on the basis of hash value
only.  The old coding can be shown to result in various bizarre,
hard-to-reproduce errors in the presence of concurrent VACUUM FULLs on
system catalogs, and could easily result in permanent catalog corruption,
up to and including complete loss of tables.

This commit is just a minimal fix that removes the unsafe comparison.
We should remove transmission of the tuple TID from sinval messages
altogether, and then arrange to suppress the extra message in the common
case of a heap_update that doesn't change the key hashvalue.  But that's
going to be much more invasive, and will only produce a probably-marginal
performance gain, so it doesn't seem like material for a back-patch.

Back-patch to 9.0.  Before that, VACUUM FULL refused to do any tuple moving
if it found any INSERT_IN_PROGRESS or DELETE_IN_PROGRESS tuples (and
CLUSTER would give up altogether), so there was no risk of moving a tuple
that might be the subject of an unsent sinval message.

diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 350e040474b413dec4280fd1e63c1f0613c5dcde..6a0c020ff97df4e7ae24fefde4fd596ec70cb6d5 100644 (file)
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -423,18 +423,19 @@ CatCacheRemoveCList(CatCache *cache, CatCList *cl)
 /*
  *     CatalogCacheIdInvalidate
  *
- *     Invalidate entries in the specified cache, given a hash value and
- *     item pointer.  Positive entries are deleted if they match the item
- *     pointer.  Negative entries must be deleted if they match the hash
- *     value (since we do not have the exact key of the tuple that's being
- *     inserted).      But this should only rarely result in loss of a cache
- *     entry that could have been kept.
+ *     Invalidate entries in the specified cache, given a hash value.
  *
- *     Note that it's not very relevant whether the tuple identified by
- *     the item pointer is being inserted or deleted.  We don't expect to
- *     find matching positive entries in the one case, and we don't expect
- *     to find matching negative entries in the other; but we will do the
- *     right things in any case.
+ *     We delete cache entries that match the hash value, whether positive
+ *     or negative.  We don't care whether the invalidation is the result
+ *     of a tuple insertion or a deletion.
+ *
+ *     We used to try to match positive cache entries by TID, but that is
+ *     unsafe after a VACUUM FULL on a system catalog: an inval event could
+ *     be queued before VACUUM FULL, and then processed afterwards, when the
+ *     target tuple that has to be invalidated has a different TID than it
+ *     did when the event was created.  So now we just compare hash values and
+ *     accept the small risk of unnecessary invalidations due to false matches.
+ *     (The ItemPointer argument is therefore useless and should get removed.)
  *
  *     This routine is only quasi-public: it should only be used by inval.c.
  */
@@ -496,11 +497,7 @@ CatalogCacheIdInvalidate(int cacheId,
 
                        nextelt = DLGetSucc(elt);
 
-                       if (hashValue != ct->hash_value)
-                               continue;               /* ignore non-matching hash values */
-
-                       if (ct->negative ||
-                               ItemPointerEquals(pointer, &ct->tuple.t_self))
+                       if (hashValue == ct->hash_value)
                        {
                                if (ct->refcount > 0 ||
                                        (ct->c_list && ct->c_list->refcount > 0))
@@ -695,12 +692,8 @@ CatalogCacheFlushCatalog(Oid catId)
 
        for (cache = CacheHdr->ch_caches; cache; cache = cache->cc_next)
        {
-               /* We can ignore uninitialized caches, since they must be empty */
-               if (cache->cc_tupdesc == NULL)
-                       continue;
-
                /* Does this cache store tuples of the target catalog? */
-               if (cache->cc_tupdesc->attrs[0]->attrelid == catId)
+               if (cache->cc_reloid == catId)
                {
                        /* Yes, so flush all its contents */
                        ResetCatalogCache(cache);