inplace130-AtEOXact_RelationCache-comments-v1.patch

Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    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