Remove AtEOXact_CatCache().

The sole useful effect of this function, to check that no catcache
entries have positive refcounts at transaction end, has really been
obsolete since we introduced ResourceOwners in PG 8.1.  We reduced the
checks to assertions years ago, so that the function was a complete
no-op in production builds.  There have been previous discussions about
removing it entirely, but consensus up to now was that it had some small
value as a cross-check for bugs in the ResourceOwner logic.

However, it now emerges that it's possible to trigger these assertions
if you hit an assert-enabled backend with SIGTERM during a call to
SearchCatCacheList, because that function temporarily increases the
refcounts of entries it's intending to add to a catcache list construct.
In a normal ERROR scenario, the extra refcounts are cleaned up by
SearchCatCacheList's PG_CATCH block; but in a FATAL exit we do a
transaction abort and exit without ever executing PG_CATCH handlers.

There's a case to be made that this is a generic hazard and we should
consider restructuring elog(FATAL) handling so that pending PG_CATCH
handlers do get run.  That's pretty scary though: it could easily create
more problems than it solves.  Preliminary stress testing by Andreas
Seltenreich suggests that there are not many live problems of this ilk,
so we rejected that idea.

There are more-localized ways to fix the problem; the most principled
one would be to use PG_ENSURE_ERROR_CLEANUP instead of plain PG_TRY.
But adding cycles to SearchCatCacheList isn't very appealing.  We could
also weaken the assertions in AtEOXact_CatCache in some more or less
ad-hoc way, but that just makes its raison d'etre even less compelling.
In the end, the most reasonable solution seems to be to just remove
AtEOXact_CatCache altogether, on the grounds that it's not worth trying
to fix it.  It hasn't found any bugs for us in many years.

Per report from Jeevan Chalke.  Back-patch to all supported branches.

Discussion: https://postgr.es/m/CAM2+6=VEE30YtRQCZX7_sCFsEpoUkFBV1gZazL70fqLn8rcvBA@mail.gmail.com

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index db90ad619b4c2467b95a4c4ba118fc6c44940b5a..03a037b978f9a3cf125e738ea053f912a0570f1b 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1987,9 +1987,6 @@ CommitTransaction(void)
                         RESOURCE_RELEASE_AFTER_LOCKS,
                         true, true);
 
-   /* Check we've released all catcache entries */
-   AtEOXact_CatCache(true);
-
    AtCommit_Notify();
    AtEOXact_GUC(true, 1);
    AtEOXact_SPI(true);
@@ -2252,9 +2249,6 @@ PrepareTransaction(void)
     */
    PostPrepare_Twophase();
 
-   /* Check we've released all catcache entries */
-   AtEOXact_CatCache(true);
-
    /* PREPARE acts the same as COMMIT as far as GUC is concerned */
    AtEOXact_GUC(true, 1);
    AtEOXact_SPI(true);
@@ -2402,7 +2396,6 @@ AbortTransaction(void)
        ResourceOwnerRelease(TopTransactionResourceOwner,
                             RESOURCE_RELEASE_AFTER_LOCKS,
                             false, true);
-       AtEOXact_CatCache(false);
 
        AtEOXact_GUC(false, 1);
        AtEOXact_SPI(false);

diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 6a27778b978c207d76afca02db015c5ac624ceaf..e4a52ce222845bff32104e3375974b5a2e693463 100644 (file)
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -542,57 +542,6 @@ CreateCacheMemoryContext(void)
 }
 
 
-/*
- *     AtEOXact_CatCache
- *
- * Clean up catcaches at end of main transaction (either commit or abort)
- *
- * As of PostgreSQL 8.1, catcache pins should get released by the
- * ResourceOwner mechanism.  This routine is just a debugging
- * cross-check that no pins remain.
- */
-void
-AtEOXact_CatCache(bool isCommit)
-{
-#ifdef USE_ASSERT_CHECKING
-   if (assert_enabled)
-   {
-       CatCache   *ccp;
-
-       for (ccp = CacheHdr->ch_caches; ccp; ccp = ccp->cc_next)
-       {
-           Dlelem     *elt;
-           int         i;
-
-           /* Check CatCLists */
-           for (elt = DLGetHead(&ccp->cc_lists); elt; elt = DLGetSucc(elt))
-           {
-               CatCList   *cl = (CatCList *) DLE_VAL(elt);
-
-               Assert(cl->cl_magic == CL_MAGIC);
-               Assert(cl->refcount == 0);
-               Assert(!cl->dead);
-           }
-
-           /* Check individual tuples */
-           for (i = 0; i < ccp->cc_nbuckets; i++)
-           {
-               for (elt = DLGetHead(&ccp->cc_bucket[i]);
-                    elt;
-                    elt = DLGetSucc(elt))
-               {
-                   CatCTup    *ct = (CatCTup *) DLE_VAL(elt);
-
-                   Assert(ct->ct_magic == CT_MAGIC);
-                   Assert(ct->refcount == 0);
-                   Assert(!ct->dead);
-               }
-           }
-       }
-   }
-#endif
-}
-
 /*
  *     ResetCatalogCache
  *

diff --git a/src/include/utils/catcache.h b/src/include/utils/catcache.h
index f932f1570dfee3a7678c39d6dfcc795adce7e2ba..1bdbeaf6b072b146153ea402cd64deb18dc5b9fe 100644 (file)
--- a/src/include/utils/catcache.h
+++ b/src/include/utils/catcache.h
@@ -162,7 +162,6 @@ typedef struct catcacheheader
 extern PGDLLIMPORT MemoryContext CacheMemoryContext;
 
 extern void CreateCacheMemoryContext(void);
-extern void AtEOXact_CatCache(bool isCommit);
 
 extern CatCache *InitCatCache(int id, Oid reloid, Oid indexoid,
             int nkeys, const int *key,