From 632ae6829f7abda34e15082c91d9dfb3fc0f298b Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 16 Aug 2011 15:26:22 -0400 Subject: [PATCH] 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. --- src/backend/utils/cache/catcache.c | 35 ++++++++++++------------------ 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c index 350e040474..6a0c020ff9 100644 --- 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); -- 2.40.0