In RelationClearRelation, postpone cache reload if !IsTransactionState().

We may process relcache flush requests during transaction startup or
shutdown.  In general it's not terribly safe to do catalog access at those
times, so the code's habit of trying to immediately revalidate unflushable
relcache entries is risky.  Although there are no field trouble reports
that are positively traceable to this, we have been able to demonstrate
failure of the assertions recently added in RelationIdGetRelation() and
SearchCatCache().  On the other hand, it seems safe to just postpone
revalidation of the cache entry until we're inside a valid transaction.
The one case where this is questionable is where we're exiting a
subtransaction and the outer transaction is holding the relcache entry open
--- but if we made any significant changes to the rel inside such a
subtransaction, we've got problems anyway.  There are mechanisms in place
to prevent that (to wit, locks for cross-session cases and
CheckTableNotInUse() for intra-session cases), so let's trust to those
mechanisms to keep us out of trouble.

Author: tglsfdc

src/backend/utils/cache/relcache.c

@@ -1601,6 +1601,7 @@ RelationIdGetRelation(Oid relationId)
 				RelationReloadIndexInfo(rd);
 			else
 				RelationClearRelation(rd, true);
+			Assert(rd->rd_isvalid);
 		}
 		return rd;
 	}
@@ -1712,8 +1713,9 @@ RelationReloadIndexInfo(Relation relation)
 	/* Should be called only for invalidated indexes */
 	Assert(relation->rd_rel->relkind == RELKIND_INDEX &&
 		   !relation->rd_isvalid);
-	/* Should be closed at smgr level */
-	Assert(relation->rd_smgr == NULL);
+
+	/* Ensure it's closed at smgr level */
+	RelationCloseSmgr(relation);
 
 	/* Must free any AM cached data upon relcache flush */
 	if (relation->rd_amcache)
@@ -1892,12 +1894,11 @@ RelationClearRelation(Relation relation, bool rebuild)
 	 * be unable to recover.  However, we must redo RelationInitPhysicalAddr
 	 * in case it is a mapped relation whose mapping changed.
 	 *
-	 * If it's a nailed index, then we need to re-read the pg_class row to see
-	 * if its relfilenode changed.	We can't necessarily do that here, because
-	 * we might be in a failed transaction.  We assume it's okay to do it if
-	 * there are open references to the relcache entry (cf notes for
-	 * AtEOXact_RelationCache).  Otherwise just mark the entry as possibly
-	 * invalid, and it'll be fixed when next opened.
+	 * If it's a nailed-but-not-mapped index, then we need to re-read the
+	 * pg_class row to see if its relfilenode changed. We do that immediately
+	 * if we're inside a valid transaction and the relation is open (not
+	 * counting the nailed refcount).  Otherwise just mark the entry as
+	 * possibly invalid, and it'll be fixed when next opened.
 	 */
 	if (relation->rd_isnailed)
 	{
@@ -1906,7 +1907,7 @@ RelationClearRelation(Relation relation, bool rebuild)
 		if (relation->rd_rel->relkind == RELKIND_INDEX)
 		{
 			relation->rd_isvalid = false;		/* needs to be revalidated */
-			if (relation->rd_refcnt > 1)
+			if (relation->rd_refcnt > 1 && IsTransactionState())
 				RelationReloadIndexInfo(relation);
 		}
 		return;
@@ -1924,7 +1925,8 @@ RelationClearRelation(Relation relation, bool rebuild)
 		relation->rd_indexcxt != NULL)
 	{
 		relation->rd_isvalid = false;	/* needs to be revalidated */
-		RelationReloadIndexInfo(relation);
+		if (IsTransactionState())
+			RelationReloadIndexInfo(relation);
 		return;
 	}
 
@@ -1945,6 +1947,29 @@ RelationClearRelation(Relation relation, bool rebuild)
 		/* And release storage */
 		RelationDestroyRelation(relation);
 	}
+	else if (!IsTransactionState())
+	{
+		/*
+		 * If we're not inside a valid transaction, we can't do any catalog
+		 * access so it's not possible to rebuild yet.  Just exit, leaving
+		 * rd_isvalid = false so that the rebuild will occur when the entry is
+		 * next opened.
+		 *
+		 * Note: it's possible that we come here during subtransaction abort,
+		 * and the reason for wanting to rebuild is that the rel is open in
+		 * the outer transaction.  In that case it might seem unsafe to not
+		 * rebuild immediately, since whatever code has the rel already open
+		 * will keep on using the relcache entry as-is.  However, in such a
+		 * case the outer transaction should be holding a lock that's
+		 * sufficient to prevent any significant change in the rel's schema,
+		 * so the existing entry contents should be good enough for its
+		 * purposes; at worst we might be behind on statistics updates or the
+		 * like.  (See also CheckTableNotInUse() and its callers.)	These same
+		 * remarks also apply to the cases above where we exit without having
+		 * done RelationReloadIndexInfo() yet.
+		 */
+		return;
+	}
 	else
 	{
 		/*
@@ -2057,6 +2082,7 @@ RelationClearRelation(Relation relation, bool rebuild)
  * RelationFlushRelation
  *
  *	 Rebuild the relation if it is open (refcount > 0), else blow it away.
+ *	 This is used when we receive a cache invalidation event for the rel.
  */
 static void
 RelationFlushRelation(Relation relation)

src/test/regress/expected/transactions.out

@@ -552,6 +552,20 @@ ROLLBACK;
 DROP TABLE foo;
 DROP TABLE baz;
 DROP TABLE barbaz;
+-- test case for problems with revalidating an open relation during abort
+create function inverse(int) returns float8 as
+$$
+begin
+  analyze revalidate_bug;
+  return 1::float8/$1;
+exception
+  when division_by_zero then return 0;
+end$$ language plpgsql volatile;
+create table revalidate_bug (c float8 unique);
+insert into revalidate_bug values (1);
+insert into revalidate_bug values (inverse(0));
+drop table revalidate_bug;
+drop function inverse(int);
 -- verify that cursors created during an aborted subtransaction are
 -- closed, but that we do not rollback the effect of any FETCHs
 -- performed in the aborted subtransaction

src/test/regress/sql/transactions.sql

@@ -333,6 +333,25 @@ DROP TABLE foo;
 DROP TABLE baz;
 DROP TABLE barbaz;
 
+
+-- test case for problems with revalidating an open relation during abort
+create function inverse(int) returns float8 as
+$$
+begin
+  analyze revalidate_bug;
+  return 1::float8/$1;
+exception
+  when division_by_zero then return 0;
+end$$ language plpgsql volatile;
+
+create table revalidate_bug (c float8 unique);
+insert into revalidate_bug values (1);
+insert into revalidate_bug values (inverse(0));
+
+drop table revalidate_bug;
+drop function inverse(int);
+
+
 -- verify that cursors created during an aborted subtransaction are
 -- closed, but that we do not rollback the effect of any FETCHs
 -- performed in the aborted subtransaction