From 58262253e167e25d4bbb6777574a98fb12850c29 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Sat, 2 Apr 2022 17:05:39 +1300
Subject: [PATCH 1/2] WIP: Rethink PROCSIGNAL_BARRIER_SMGRRELEASE.

With sufficiently bad luck, it was possible for IssuePendingWritebacks()
to reopen relation files just after we'd processed
PROCSIGNAL_BARRIER_SMGRRELEASE but before the file was unlinked by some
other backend.

While commit 4eb21763 initially appeared to have fixed the spate of test
failures we'd seen on Windows, testing with extra instrumentation to
look out for writes to unlinked files revealed the dangerous nature of
these deferred file references.

This second attempt introduces smgrrelease(reln), smgrreleaseall(), and
smgrreleaseallcount().  That last is used by IssuedPendingWritebacks()
to notice when pending writebacks have to be consider potentially
invalid, by watching out for level changes during the time writeback
instructions were accumulated.

XXX Experimental

Reported-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20220209220004.kb3dgtn2x2k2gtdm%40alap3.anarazel.de
---
 src/backend/storage/buffer/bufmgr.c | 14 +++++++
 src/backend/storage/smgr/md.c       |  6 ---
 src/backend/storage/smgr/smgr.c     | 58 +++++++++++++++++++++++++----
 src/include/storage/buf_internals.h |  3 ++
 src/include/storage/md.h            |  1 -
 src/include/storage/smgr.h          |  3 ++
 6 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index d73a40c1bc..43652b757a 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4873,6 +4873,7 @@ WritebackContextInit(WritebackContext *context, int *max_pending)
 
 	context->max_pending = max_pending;
 	context->nr_pending = 0;
+	context->smgr_release_count = smgrreleaseallcount();
 }
 
 /*
@@ -4924,6 +4925,18 @@ IssuePendingWritebacks(WritebackContext *context)
 {
 	int			i;
 
+	/*
+	 * Throw away the contents if smgrreleaseall() has been invoked since
+	 * "context" was initialized or cleared.  Since the main loop below doesn't
+	 * check for interrupts or otherwise handle barrier requests, we don't have
+	 * to worry about invalidation if we get past this check.
+	 */
+	if (context->smgr_release_count != smgrreleaseallcount())
+	{
+		context->nr_pending = 0;
+		context->smgr_release_count = smgrreleaseallcount();
+	}
+
 	if (context->nr_pending == 0)
 		return;
 
@@ -4983,6 +4996,7 @@ IssuePendingWritebacks(WritebackContext *context)
 	}
 
 	context->nr_pending = 0;
+	context->smgr_release_count = smgrreleaseallcount();
 }
 
 
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 879f647dbc..d26c915f90 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -549,12 +549,6 @@ mdclose(SMgrRelation reln, ForkNumber forknum)
 	}
 }
 
-void
-mdrelease(void)
-{
-	closeAllVfds();
-}
-
 /*
  *	mdprefetch() -- Initiate asynchronous read of the specified block of a relation
  */
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index d71a557a35..087c8e76d1 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -41,7 +41,6 @@ typedef struct f_smgr
 {
 	void		(*smgr_init) (void);	/* may be NULL */
 	void		(*smgr_shutdown) (void);	/* may be NULL */
-	void		(*smgr_release) (void); /* may be NULL */
 	void		(*smgr_open) (SMgrRelation reln);
 	void		(*smgr_close) (SMgrRelation reln, ForkNumber forknum);
 	void		(*smgr_create) (SMgrRelation reln, ForkNumber forknum,
@@ -70,7 +69,6 @@ static const f_smgr smgrsw[] = {
 	{
 		.smgr_init = mdinit,
 		.smgr_shutdown = NULL,
-		.smgr_release = mdrelease,
 		.smgr_open = mdopen,
 		.smgr_close = mdclose,
 		.smgr_create = mdcreate,
@@ -100,6 +98,8 @@ static dlist_head unowned_relns;
 /* local function prototypes */
 static void smgrshutdown(int code, Datum arg);
 
+static uint64 releaseall_count = 0;
+
 
 /*
  *	smgrinit(), smgrshutdown() -- Initialize or shut down storage
@@ -281,6 +281,44 @@ smgrclose(SMgrRelation reln)
 		*owner = NULL;
 }
 
+/*
+ *	smgrrelease() -- Release all resources used by this object.
+ *
+ *	The object remains valid.
+ */
+void
+smgrrelease(SMgrRelation reln)
+{
+	for (int i = 0; i <= MAX_FORKNUM; ++i)
+	{
+		smgrsw[reln->smgr_which].smgr_close(reln, i);
+		reln->smgr_cached_nblocks[i] = InvalidBlockNumber;
+	}
+}
+
+/*
+ *	smgrreleaseall() -- Release resources used by all objects.
+ *
+ *	This is called for PROCSIGNAL_BARRIER_SMGRRELEASE.
+ */
+void
+smgrreleaseall(void)
+{
+	HASH_SEQ_STATUS status;
+	SMgrRelation reln;
+
+	/* Nothing to do if hashtable not set up */
+	if (SMgrRelationHash == NULL)
+		return;
+
+	hash_seq_init(&status, SMgrRelationHash);
+
+	while ((reln = (SMgrRelation) hash_seq_search(&status)) != NULL)
+		smgrrelease(reln);
+
+	releaseall_count++;
+}
+
 /*
  *	smgrcloseall() -- Close all existing SMgrRelation objects.
  */
@@ -696,6 +734,15 @@ AtEOXact_SMgr(void)
 	}
 }
 
+/*
+ * How many times has smgrreleaseall() run in this backend?
+ */
+uint64
+smgrreleaseallcount(void)
+{
+	return releaseall_count;
+}
+
 /*
  * This routine is called when we are ordered to release all open files by a
  * ProcSignalBarrier.
@@ -703,11 +750,6 @@ AtEOXact_SMgr(void)
 bool
 ProcessBarrierSmgrRelease(void)
 {
-	for (int i = 0; i < NSmgr; i++)
-	{
-		if (smgrsw[i].smgr_release)
-			smgrsw[i].smgr_release();
-	}
-
+	smgrreleaseall();
 	return true;
 }
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index b903d2bcaf..b2b8856a34 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -270,6 +270,9 @@ typedef struct WritebackContext
 	/* current number of pending writeback requests */
 	int			nr_pending;
 
+	/* smgrrelease() count for this backend, before anything is scheduled */
+	uint64		smgr_release_count;
+
 	/* pending requests */
 	PendingWriteback pending_writebacks[WRITEBACK_MAX_PENDING_FLUSHES];
 } WritebackContext;
diff --git a/src/include/storage/md.h b/src/include/storage/md.h
index 6e46d8d96a..ffffa40db7 100644
--- a/src/include/storage/md.h
+++ b/src/include/storage/md.h
@@ -23,7 +23,6 @@
 extern void mdinit(void);
 extern void mdopen(SMgrRelation reln);
 extern void mdclose(SMgrRelation reln, ForkNumber forknum);
-extern void mdrelease(void);
 extern void mdcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo);
 extern bool mdexists(SMgrRelation reln, ForkNumber forknum);
 extern void mdunlink(RelFileNodeBackend rnode, ForkNumber forknum, bool isRedo);
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 8e3ef92cda..4661971003 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -85,6 +85,9 @@ extern void smgrclearowner(SMgrRelation *owner, SMgrRelation reln);
 extern void smgrclose(SMgrRelation reln);
 extern void smgrcloseall(void);
 extern void smgrclosenode(RelFileNodeBackend rnode);
+extern void smgrrelease(SMgrRelation reln);
+extern void smgrreleaseall(void);
+extern uint64 smgrreleaseallcount(void);
 extern void smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo);
 extern void smgrdosyncall(SMgrRelation *rels, int nrels);
 extern void smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo);
-- 
2.35.1

