Author: Noah Misch Commit: Noah Misch Fix comments in and referring to AtEOXact_RelationCache(). The first point has prompted this change. We can fix a bug by calling AtEOXact_Inval(true) earlier, in the COMMIT critical section. Remove comment text that would have required AtEOXact_RelationCache() to move with it. Full list of defects fixed: - Commit 8de3e410faa06ab20ec1aa6d0abb0a2c040261ba (2014-02) made relcache.c defer rebuilds if !IsTransactionState(). That removed the functional implications of swapping the order of AtEOXact_RelationCache() and AtEOXact_Inval(). Even without that change, we'd have other layers of defense. At COMMIT: - Code that opened rels already closed them. - AtEOXact_RelationCache() essentially sets some non-pointer fields and calls RelationDestroyRelation() to pfree() rels for which we're committing both a CREATE and a DROP. None of that needs a transaction. - AtEOXact_Inval() doesn't locally-execute messages. The next AcceptInvalidationMessages() does that. At ABORT: - resowner.c already closed all rels. - AtEOXact_RelationCache() essentially sets some non-pointer fields and calls RelationDestroyRelation() to pfree() rels for which we're aborting a CREATE. - Commit f884dca4959f64bd47e78102d1dadd2c77d49201 (2019-05) removed "forced index lists". - The header comment listed just a subset of the function's activities. Commit fdd965d074d46765c295223b119ca437dbcac973 (back-patched to v9.6) added in_progress_list cleanup. Commit e5550d5fec66aa74caad1f79b79826ec64898688 (2014-04) added EOXactTupleDescArray cleanup. - When commit d5b31cc32b0762fa8920a9c1f70af2f82fb0aaa5 (2013-01) introduced the eoxact_list mechanism, the function ceased crashing when called before initialization. - While "current transaction created any relations" is the activity with the most code here, that appeared at the end of a paragraph that started with pre-8.1 behavior. Back-patch to v12 (all supported versions), the plan for calling AtEOXact_Inval(true) earlier. Reviewed by FIXME. Discussion: https://postgr.es/m/20240523000548.58.nmisch@google.com diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 9bda1aa..ee389fc 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -2369,11 +2369,10 @@ CommitTransaction(void) AtEOXact_RelationCache(true); /* - * Make catalog changes visible to all backends. This has to happen after - * relcache references are dropped (see comments for - * AtEOXact_RelationCache), but before locks are released (if anyone is - * waiting for lock on a relation we've modified, we want them to know - * about the catalog change before they start using the relation). + * Make catalog changes visible to all backends. This has to happen + * before locks are released (if anyone is waiting for lock on a relation + * we've modified, we want them to know about the catalog change before + * they start using the relation). */ AtEOXact_Inval(true); diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index cbf9cf2..ed4a8eb 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -3218,17 +3218,6 @@ AssertPendingSyncs_RelationCache(void) * AtEOXact_RelationCache * * Clean up the relcache at main-transaction commit or abort. - * - * Note: this must be called *before* processing invalidation messages. - * In the case of abort, we don't want to try to rebuild any invalidated - * cache entries (since we can't safely do database accesses). Therefore - * we must reset refcnts before handling pending invalidations. - * - * As of PostgreSQL 8.1, relcache refcnts should get released by the - * ResourceOwner mechanism. This routine just does a debugging - * cross-check that no pins remain. However, we also need to do special - * cleanup when the current transaction created any relations or made use - * of forced index lists. */ void AtEOXact_RelationCache(bool isCommit) @@ -3245,8 +3234,9 @@ AtEOXact_RelationCache(bool isCommit) in_progress_list_len = 0; /* - * Unless the eoxact_list[] overflowed, we only need to examine the rels - * listed in it. Otherwise fall back on a hash_seq_search scan. + * Cleanup rels created in the current transaction. Unless the + * eoxact_list[] overflowed, we only need to examine the rels listed in + * it. Otherwise fall back on a hash_seq_search scan. * * For simplicity, eoxact_list[] entries are not deleted till end of * top-level transaction, even though we could remove them at @@ -3307,7 +3297,8 @@ AtEOXact_cleanup(Relation relation, bool isCommit) /* * The relcache entry's ref count should be back to its normal - * not-in-a-transaction state: 0 unless it's nailed in cache. + * not-in-a-transaction state: 0 unless it's nailed in cache. The + * ResourceOwner mechanism handles that. * * In bootstrap mode, this is NOT true, so don't check it --- the * bootstrap code expects relations to stay open across start/commit @@ -3982,10 +3973,7 @@ RelationAssumeNewRelfilelocator(Relation relation) * This initializes the relation descriptor cache. At the time * that this is invoked, we can't do database access yet (mainly * because the transaction subsystem is not up); all we are doing - * is making an empty cache hashtable. This must be done before - * starting the initialization transaction, because otherwise - * AtEOXact_RelationCache would crash if that transaction aborts - * before we can get the relcache set up. + * is making an empty cache hashtable. */ #define INITRELCACHESIZE 400