v2-0002-RelationTruncate-must-use-a-critical-section.patch

From 790dc839ac0d3c48ebd10c7a50f3b84cf8ed4fd0 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Sun, 21 Apr 2024 09:33:35 +1200
Subject: [PATCH v2 2/2] RelationTruncate() must use a critical section.

RelationTruncate() does three things, while holding an
AccessExclusiveLock and preventing checkpoints:

1. Logs the truncation.
2. Drops buffers, even if they're dirty.
3. Truncates files, possibly more than one.

Step 3 might fail with an operating system error, but we're already
committed to the operation at this point.

Previously the theory expressed in comments was that crash recovery
would surely fail to truncate again, so we should avoid a PANIC loop.
That didn't account for the seriousness of the failure mode: dead tuples
could come back to life, because we threw away needed dirty buffers, and
replicas could PANIC, because they replayed the truncation but then
later they might see references to blocks they consider to be past the
end.

This commit wraps the whole operation in a critical section, so that any
error triggers a PANIC.  In order to make that possible, it provides
smgrpreparetruncate() so that smgrtruncate() can promise not to call
palloc().  Otherwise, the rule about not allocating memory in a critical
section would be broken.  This in turn requires teaching relpath.c to
build paths in place.  In theory this introduces a new failure mode
where a path is detected as being too long for MAXPGPATH, but that was
already likely to break.

This addresses bug #18146, an ancient problem mostly affecting Windows
in practice, but also bug #18426 which revealed a new way to reach a
partially truncated state even on Unix since PostgreSQL 14.  Commit
d8725104 made it possible for WaitIO() reached via DropRelationBuffers()
to be interrupted, but that's no longer possible as a side-effect of the
critical section.

This issue has been independently analyzed by a large number of people
and threads over the years, most recently Robert Haas and myself.  Ideas
for how to fix with new buffer states have been proposed but that
wouldn't be back-patchable.

XXX Seems a bit much to back-patch!

Discussion: https://postgr.es/m/18146-04e908c662113ad5%40postgresql.org
Discussion: https://postgr.es/m/2348.1544474335@sss.pgh.pa.us
Discussion: https://postgr.es/m/5BBC590AE8DF4ED1A170E4D48F1B53AC%40tunaPC
---
 src/backend/catalog/storage.c   | 28 +++++++++---
 src/backend/storage/smgr/md.c   | 75 +++++++++++++++++++++------------
 src/backend/storage/smgr/smgr.c | 12 ++++++
 src/common/relpath.c            | 57 +++++++++++++++++++------
 src/include/common/relpath.h    |  3 ++
 src/include/storage/smgr.h      |  1 +
 6 files changed, 132 insertions(+), 44 deletions(-)

diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index bb8c4d15612..dabaa48a57b 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -308,6 +308,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
 	forks[nforks] = MAIN_FORKNUM;
 	blocks[nforks] = nblocks;
 	nforks++;
+	smgrpreparetruncate(reln, MAIN_FORKNUM);
 
 	/* Prepare for truncation of the FSM if it exists */
 	fsm = smgrexists(RelationGetSmgr(rel), FSM_FORKNUM);
@@ -320,6 +321,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
 			nforks++;
 			need_fsm_vacuum = true;
 		}
+		smgrpreparetruncate(reln, FSM_FORKNUM);
 	}
 
 	/* Prepare for truncation of the visibility map too if it exists */
@@ -332,6 +334,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
 			forks[nforks] = VISIBILITYMAP_FORKNUM;
 			nforks++;
 		}
+		smgrpreparetruncate(reln, VISIBILITYMAP_FORKNUM);
 	}
 
 	RelationPreTruncate(rel);
@@ -368,12 +371,25 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
 	/*
 	 * We WAL-log the truncation before actually truncating, which means
 	 * trouble if the truncation fails. If we then crash, the WAL replay
-	 * likely isn't going to succeed in the truncation either, and cause a
-	 * PANIC. It's tempting to put a critical section here, but that cure
-	 * would be worse than the disease. It would turn a usually harmless
-	 * failure to truncate, that might spell trouble at WAL replay, into a
-	 * certain PANIC.
+	 * likely isn't going to succeed in the truncation either, so it's
+	 * tempting not to put a critical section here to avoid a double PANIC.
+	 * However, if the file system operation failed after
+	 * DropRelationBuffers() had already thrown away dirty buffers, (1) old
+	 * deleted data might later come back to life, and (2) would-be-truncated
+	 * pages might be modified again, and then a replica that had replayed the
+	 * truncation would PANIC because it has a different idea of the relation
+	 * size.
+	 *
+	 * The critical section also makes WaitIO() non-interruptable in
+	 * DropRelationBuffers(), which would otherwise be another way to reach
+	 * the above problems.
+	 *
+	 * Note that we called smgrpreparetruncate() above, to allow
+	 * smgrtruncate() to be called within a critical section without
+	 * allocating memory.
 	 */
+	START_CRIT_SECTION();
+
 	if (RelationNeedsWAL(rel))
 	{
 		/*
@@ -410,6 +426,8 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
 	 */
 	smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks);
 
+	END_CRIT_SECTION();
+
 	/* We've done all the critical work, so checkpoints are OK now. */
 	MyProc->delayChkptFlags &= ~(DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE);
 
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index bf0f3ca76d1..139cb217cc2 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -117,6 +117,12 @@ static MemoryContext MdCxt;		/* context for all MdfdVec objects */
 /* don't try to open a segment, if not already open */
 #define EXTENSION_DONT_OPEN			(1 << 5)
 
+/*
+ * The maximum possible size of the .NNN extension for segments is 1 for the
+ * dot plus log10(2^32) for the digits for RELSEG_SIZE = 1, and it's not worth
+ * working harder to compute the value for more typical RELSEG_SIZE values.
+ */
+#define MAX_EXTENSION_SIZE			(1 + 10)
 
 /* local routines */
 static void mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum,
@@ -131,7 +137,7 @@ static void register_forget_request(RelFileLocatorBackend rlocator, ForkNumber f
 static void _fdvec_resize(SMgrRelation reln,
 						  ForkNumber forknum,
 						  int nseg);
-static char *_mdfd_segpath(SMgrRelation reln, ForkNumber forknum,
+static char *_mdfd_segpath(char *path, SMgrRelation reln, ForkNumber forknum,
 						   BlockNumber segno);
 static MdfdVec *_mdfd_openseg(SMgrRelation reln, ForkNumber forknum,
 							  BlockNumber segno, int oflags);
@@ -408,7 +414,8 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo)
 	 */
 	if (ret >= 0 || errno != ENOENT)
 	{
-		char	   *segpath = (char *) palloc(strlen(path) + 12);
+		char	   *segpath = (char *) palloc(strlen(path) +
+											  MAX_EXTENSION_SIZE + 1);
 		BlockNumber segno;
 
 		for (segno = 1;; segno++)
@@ -1492,7 +1499,7 @@ _fdvec_resize(SMgrRelation reln,
 		reln->md_seg_fds[forknum] =
 			MemoryContextAlloc(MdCxt, sizeof(MdfdVec) * nseg);
 	}
-	else
+	else if (nseg > reln->md_num_open_segs[forknum])
 	{
 		/*
 		 * It doesn't seem worthwhile complicating the code to amortize
@@ -1504,31 +1511,50 @@ _fdvec_resize(SMgrRelation reln,
 			repalloc(reln->md_seg_fds[forknum],
 					 sizeof(MdfdVec) * nseg);
 	}
+	else
+	{
+		/*
+		 * We don't reallocate a smaller array, because we want
+		 * smgrpreparetruncate() to be able to promise that smgrtruncate()
+		 * won't call memory allocation functions that would fail in a
+		 * critical section.  This means that a bit of space in the array is
+		 * now wasted, until the next time we add a segment and reallocate.
+		 */
+	}
 
 	reln->md_num_open_segs[forknum] = nseg;
 }
 
 /*
- * Return the filename for the specified segment of the relation. The
- * returned string is palloc'd.
+ * Return the filename for the specified segment of the relation.  The output
+ * buffer must be of size MAXPGPATH.
  */
 static char *
-_mdfd_segpath(SMgrRelation reln, ForkNumber forknum, BlockNumber segno)
+_mdfd_segpath(char *path, SMgrRelation reln, ForkNumber forknum, BlockNumber segno)
 {
-	char	   *path,
-			   *fullpath;
+	size_t		size;
 
-	path = relpath(reln->smgr_rlocator, forknum);
+	size = GetRelationPathInPlace(path,
+								  reln->smgr_rlocator.locator.dbOid,
+								  reln->smgr_rlocator.locator.spcOid,
+								  reln->smgr_rlocator.locator.relNumber,
+								  reln->smgr_rlocator.backend,
+								  forknum);
 
+	/*
+	 * Make sure the whole path, maximum possible segment suffix and NUL
+	 * terminator can fit into MAXPGPATH.
+	 */
+	if (size + MAX_EXTENSION_SIZE >= MAXPGPATH)
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("path \"%s\" too long", path)));
+
+	/* Append segment number. */
 	if (segno > 0)
-	{
-		fullpath = psprintf("%s.%u", path, segno);
-		pfree(path);
-	}
-	else
-		fullpath = path;
+		sprintf(path + size, ".%u", segno);
 
-	return fullpath;
+	return path;
 }
 
 /*
@@ -1541,15 +1567,13 @@ _mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno,
 {
 	MdfdVec    *v;
 	File		fd;
-	char	   *fullpath;
+	char		fullpath[MAXPGPATH];
 
-	fullpath = _mdfd_segpath(reln, forknum, segno);
+	_mdfd_segpath(fullpath, reln, forknum, segno);
 
 	/* open the file */
 	fd = PathNameOpenFile(fullpath, _mdfd_open_flags() | oflags);
 
-	pfree(fullpath);
-
 	if (fd < 0)
 		return NULL;
 
@@ -1584,6 +1608,7 @@ static MdfdVec *
 _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
 			 bool skipFsync, int behavior)
 {
+	char		path[MAXPGPATH];
 	MdfdVec    *v;
 	BlockNumber targetseg;
 	BlockNumber nextsegno;
@@ -1686,7 +1711,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
 			ereport(ERROR,
 					(errcode_for_file_access(),
 					 errmsg("could not open file \"%s\" (target block %u): previous segment is only %u blocks",
-							_mdfd_segpath(reln, forknum, nextsegno),
+							_mdfd_segpath(path, reln, forknum, nextsegno),
 							blkno, nblocks)));
 		}
 
@@ -1700,7 +1725,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
 			ereport(ERROR,
 					(errcode_for_file_access(),
 					 errmsg("could not open file \"%s\" (target block %u): %m",
-							_mdfd_segpath(reln, forknum, nextsegno),
+							_mdfd_segpath(path, reln, forknum, nextsegno),
 							blkno)));
 		}
 	}
@@ -1751,11 +1776,7 @@ mdsyncfiletag(const FileTag *ftag, char *path)
 	}
 	else
 	{
-		char	   *p;
-
-		p = _mdfd_segpath(reln, ftag->forknum, ftag->segno);
-		strlcpy(path, p, MAXPGPATH);
-		pfree(p);
+		_mdfd_segpath(path, reln, ftag->forknum, ftag->segno);
 
 		file = PathNameOpenFile(path, _mdfd_open_flags());
 		if (file < 0)
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 100f6454e53..78f562034ec 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -689,6 +689,18 @@ smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum)
 	return InvalidBlockNumber;
 }
 
+/*
+ * smgrpreparetruncate() -- Prepare to truncate a fork of a relation.
+ *
+ * This promises that a later call to smgrtruncate() will not have to allocate
+ * memory from MemoryContexts that don't allow that inside critical sections.
+ */
+void
+smgrpreparetruncate(SMgrRelation reln, ForkNumber forknum)
+{
+	smgrnblocks(reln, forknum);
+}
+
 /*
  * smgrtruncate() -- Truncate the given forks of supplied relation to
  *					 each specified numbers of blocks
diff --git a/src/common/relpath.c b/src/common/relpath.c
index f54c36ef7ac..7974a9cb0ee 100644
--- a/src/common/relpath.c
+++ b/src/common/relpath.c
@@ -141,7 +141,32 @@ char *
 GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
 				int procNumber, ForkNumber forkNumber)
 {
-	char	   *path;
+	char		path[MAXPGPATH];
+	char	   *result;
+	size_t		size;
+
+	size = GetRelationPathInPlace(path, dbOid, spcOid, relNumber, procNumber, forkNumber);
+
+	/* String plus NUL terminator. */
+	result = palloc(size + 1);
+	memcpy(result, path, size + 1);
+
+	return result;
+}
+
+/*
+ * GetRelationPathInPlace - construct path to a relation's file
+ *
+ * Result is written to path, which must have space for MAXPGPATH characters.
+ * The size of the string is returned.  If it is >= MAXPGPATH, the path has
+ * been truncated due to lack of space.
+ */
+size_t
+GetRelationPathInPlace(char *path,
+					   Oid dbOid, Oid spcOid, RelFileNumber relNumber,
+					   int procNumber, ForkNumber forkNumber)
+{
+	size_t		size;
 
 	if (spcOid == GLOBALTABLESPACE_OID)
 	{
@@ -149,10 +174,10 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
 		Assert(dbOid == 0);
 		Assert(procNumber == INVALID_PROC_NUMBER);
 		if (forkNumber != MAIN_FORKNUM)
-			path = psprintf("global/%u_%s",
+			size = snprintf(path, MAXPGPATH, "global/%u_%s",
 							relNumber, forkNames[forkNumber]);
 		else
-			path = psprintf("global/%u", relNumber);
+			size = snprintf(path, MAXPGPATH, "global/%u", relNumber);
 	}
 	else if (spcOid == DEFAULTTABLESPACE_OID)
 	{
@@ -160,21 +185,25 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
 		if (procNumber == INVALID_PROC_NUMBER)
 		{
 			if (forkNumber != MAIN_FORKNUM)
-				path = psprintf("base/%u/%u_%s",
+				size = snprintf(path, MAXPGPATH,
+								"base/%u/%u_%s",
 								dbOid, relNumber,
 								forkNames[forkNumber]);
 			else
-				path = psprintf("base/%u/%u",
+				size = snprintf(path, MAXPGPATH,
+								"base/%u/%u",
 								dbOid, relNumber);
 		}
 		else
 		{
 			if (forkNumber != MAIN_FORKNUM)
-				path = psprintf("base/%u/t%d_%u_%s",
+				size = snprintf(path, MAXPGPATH,
+								"base/%u/t%d_%u_%s",
 								dbOid, procNumber, relNumber,
 								forkNames[forkNumber]);
 			else
-				path = psprintf("base/%u/t%d_%u",
+				size = snprintf(path, MAXPGPATH,
+								"base/%u/t%d_%u",
 								dbOid, procNumber, relNumber);
 		}
 	}
@@ -184,27 +213,31 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
 		if (procNumber == INVALID_PROC_NUMBER)
 		{
 			if (forkNumber != MAIN_FORKNUM)
-				path = psprintf("pg_tblspc/%u/%s/%u/%u_%s",
+				size = snprintf(path, MAXPGPATH,
+								"pg_tblspc/%u/%s/%u/%u_%s",
 								spcOid, TABLESPACE_VERSION_DIRECTORY,
 								dbOid, relNumber,
 								forkNames[forkNumber]);
 			else
-				path = psprintf("pg_tblspc/%u/%s/%u/%u",
+				size = snprintf(path, MAXPGPATH,
+								"pg_tblspc/%u/%s/%u/%u",
 								spcOid, TABLESPACE_VERSION_DIRECTORY,
 								dbOid, relNumber);
 		}
 		else
 		{
 			if (forkNumber != MAIN_FORKNUM)
-				path = psprintf("pg_tblspc/%u/%s/%u/t%d_%u_%s",
+				size = snprintf(path, MAXPGPATH,
+								"pg_tblspc/%u/%s/%u/t%d_%u_%s",
 								spcOid, TABLESPACE_VERSION_DIRECTORY,
 								dbOid, procNumber, relNumber,
 								forkNames[forkNumber]);
 			else
-				path = psprintf("pg_tblspc/%u/%s/%u/t%d_%u",
+				size = snprintf(path, MAXPGPATH,
+								"pg_tblspc/%u/%s/%u/t%d_%u",
 								spcOid, TABLESPACE_VERSION_DIRECTORY,
 								dbOid, procNumber, relNumber);
 		}
 	}
-	return path;
+	return size;
 }
diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
index 6f006d5a938..dfab46c0037 100644
--- a/src/include/common/relpath.h
+++ b/src/include/common/relpath.h
@@ -75,6 +75,9 @@ extern char *GetDatabasePath(Oid dbOid, Oid spcOid);
 
 extern char *GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
 							 int procNumber, ForkNumber forkNumber);
+extern size_t GetRelationPathInPlace(char *path,
+									 Oid dbOid, Oid spcOid, RelFileNumber relNumber,
+									 int procNumber, ForkNumber forkNumber);
 
 /*
  * Wrapper macros for GetRelationPath.  Beware of multiple
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index fc5f883ce14..77974b0f36f 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -103,6 +103,7 @@ extern void smgrwriteback(SMgrRelation reln, ForkNumber forknum,
 						  BlockNumber blocknum, BlockNumber nblocks);
 extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum);
 extern BlockNumber smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum);
+extern void smgrpreparetruncate(SMgrRelation reln, ForkNumber forknum);
 extern void smgrtruncate(SMgrRelation reln, ForkNumber *forknum,
 						 int nforks, BlockNumber *nblocks);
 extern void smgrimmedsync(SMgrRelation reln, ForkNumber forknum);
-- 
2.44.0