8000 Remove AtEOXact_CatCache(). · softliumin/postgres@425be3a · GitHub
[go: up one dir, main page]
Skip to content

Commit 425be3a

Browse files
tglsfdctglsfdc
committed
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
1 parent d1c1d90 commit 425be3a

File tree

3 files changed

+0
-59
lines changed

3 files changed

+0
-59
lines changed

‎src/backend/access/transam/xact.c

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2106,9 +2106,6 @@ CommitTransaction(void)
21062106
*/
21072107
smgrDoPendingDeletes(true);
21082108

2109-
/* Check we've released all catcache entries */
2110-
AtEOXact_CatCache(true);
2111-
21122109
AtCommit_Notify();
21132110
AtEOXact_GUC(true, 1);
21142111
AtEOXact_SPI(true);
@@ -2377,9 +2374,6 @@ PrepareTransaction(void)
23772374
*/
23782375
PostPrepare_Twophase();
23792376

2380-
/* Check we've released all catcache entries */
2381-
AtEOXact_CatCache(true);
2382-
23832377
/* PREPARE acts the same as COMMIT as far as GUC is concerned */
23842378
AtEOXact_GUC(true, 1);
23852379
AtEOXact_SPI(true);
@@ -2575,7 +2569,6 @@ AbortTransaction(void)
25752569
RESOURCE_RELEASE_AFTER_LOCKS,
25762570
false, true);
25772571
smgrDoPendingDeletes(false);
2578-
AtEOXact_CatCache(false);
25792572

25802573
AtEOXact_GUC(false, 1);
25812574
AtEOXact_SPI(false);

‎src/backend/utils/cache/catcache.c

Lines changed: 0 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -528,57 +528,6 @@ CreateCacheMemoryContext(void)
528528
}
529529

530530

531-
/*
532-
* AtEOXact_CatCache
533-
*
534-
* Clean up catcaches at end of main transaction (either commit or abort)
535-
*
536-
* As of PostgreSQL 8.1, catcache pins should get released by the
537-
* ResourceOwner mechanism. This routine is just a debugging
538-
* cross-check that no pins remain.
539-
*/
540-
void
541-
AtEOXact_CatCache(bool isCommit)
542-
{
543-
#ifdef USE_ASSERT_CHECKING
544-
slist_iter cache_iter;
545-
546-
slist_foreach(cache_iter, &CacheHdr->ch_caches)
547-
{
548-
CatCache *ccp = slist_container(CatCache, cc_next, cache_iter.cur);
549-
dlist_iter iter;
550-
int i;
551-
552-
/* Check CatCLists */
553-
dlist_foreach(iter, &ccp->cc_lists)
554-
{
555-
CatCList *cl;
556-
557-
cl = dlist_container(CatCList, cache_elem, iter.cur);
558-
Assert(cl->cl_magic == CL_MAGIC);
559-
Assert(cl->refcount == 0);
560-
Assert(!cl->dead);
561-
}
562-
563-
/* Check individual tuples */
564-
for (i = 0; i < ccp->cc_nbuckets; i++)
565-
{
566-
dlist_head *bucket = &ccp->cc_bucket[i];
567-
568-
dlist_foreach(iter, bucket)
569-
{
570-
CatCTup *ct;
571-
572-
ct = dlist_container(CatCTup, cache_elem, iter.cur);
573-
Assert(ct->ct_magic == CT_MAGIC);
574-
Assert(ct->refcount == 0);
575-
Assert(!ct->dead);
576-
}
577-
}
578-
}
579-
#endif
580-
}
581-
582531
/*
583532
* ResetCatalogCache
584533
*

‎src/include/utils/catcache.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,6 @@ typedef struct catcacheheader
162162
extern PGDLLIMPORT MemoryContext CacheMemoryContext;
163163

164164
extern void CreateCacheMemoryContext(void);
165-
extern void AtEOXact_CatCache(bool isCommit);
166165

167166
extern CatCache *InitCatCache(int id, Oid reloid, Oid indexoid,
168167
int nkeys, const int *key,

0 commit comments

Comments
 (0)
0