v5-0001-Fix-corruption-on-failed-interrupted-truncation.patch

From c0e74c507483c97297ed66cdfb9ec2e16f0dde3f Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 10 Sep 2024 12:04:38 +1200
Subject: [PATCH v5 1/2] Fix corruption on failed/interrupted truncation.

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 reanimated 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
smgrtruncatefrom(), so that we can move the smgrnblocks() calls out of
the critical section, and adjusts md.c to defer reallocating its
internal array of segments when its size decreases.

This addresses bug #18146, an ancient problem mostly affecting Windows
in practice.  ftruncate() failed only in very rare circumstances on Unix
(but it could!), while on Windows there seem to be a more reports of
transient failures (really chsize() or more likely the reopen in
FileAccess(), for which a root cause has not been determined).

It also addresses bug #18426, which revealed a new way to reach these
corrupted partially truncated states even on Unix since PostgreSQL 14.
Commit d8725104 made it possible for WaitIO(), reachable via
DropRelationBuffers(), to be interrupted.  That's no longer possible as
a side-effect of the new critical section.

The equivalent smgrtruncate() call in recovery doesn't need a critical
section, because both types of errors would be promoted to FATAL in the
startup process (it has no error handler), and then treated much like a
PANIC by the postmaster.

Alternatives involving new buffer states for pending truncation, and
other ways of making smgrtruncate() safe in critical sections have been
mooted, but this approach seems the simplest.  Unfortunately it involves
back-patching a change to the smgr interface.  That's private but it
might be a configuration point in fork projects.  We'd prefer to avoid
that but we agreed it was acceptable to fix a data corruption bug.

The ftruncate() failure has been independently analyzed by a large
number of people and threads over the years, and the WaitIO()
cancellation variant was more recently diagnosed by Alexander Lakhin.

XXX shouldn't we flush the WAL unconditionally?

Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Diagnosed-by: Alexander Lakhin <exclusion@gmail.com>
Reported-by: rootcause000@gmail.com
Reported-by: Alexander Lakhin <exclusion@gmail.com>
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   | 31 +++++++++++++++++++++++++------
 src/backend/storage/smgr/md.c   | 27 +++++++++++++++++++--------
 src/backend/storage/smgr/smgr.c | 31 +++++++++++++++++++++++++++++--
 src/include/storage/md.h        |  2 +-
 src/include/storage/smgr.h      |  3 +++
 5 files changed, 77 insertions(+), 17 deletions(-)

diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index bb8c4d15612..a14f45c4cff 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -291,6 +291,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
 	bool		vm;
 	bool		need_fsm_vacuum = false;
 	ForkNumber	forks[MAX_FORKNUM];
+	BlockNumber old_blocks[MAX_FORKNUM];
 	BlockNumber blocks[MAX_FORKNUM];
 	int			nforks = 0;
 	SMgrRelation reln;
@@ -306,6 +307,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
 
 	/* Prepare for truncation of MAIN fork of the relation */
 	forks[nforks] = MAIN_FORKNUM;
+	old_blocks[nforks] = smgrnblocks(reln, MAIN_FORKNUM);
 	blocks[nforks] = nblocks;
 	nforks++;
 
@@ -316,6 +318,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
 		blocks[nforks] = FreeSpaceMapPrepareTruncateRel(rel, nblocks);
 		if (BlockNumberIsValid(blocks[nforks]))
 		{
+			old_blocks[nforks] = smgrnblocks(reln, FSM_FORKNUM);
 			forks[nforks] = FSM_FORKNUM;
 			nforks++;
 			need_fsm_vacuum = true;
@@ -329,6 +332,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
 		blocks[nforks] = visibilitymap_prepare_truncate(rel, nblocks);
 		if (BlockNumberIsValid(blocks[nforks]))
 		{
+			old_blocks[nforks] = smgrnblocks(reln, VISIBILITYMAP_FORKNUM);
 			forks[nforks] = VISIBILITYMAP_FORKNUM;
 			nforks++;
 		}
@@ -368,12 +372,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-interruptible in
+	 * DropRelationBuffers(), which would otherwise be another way to reach
+	 * the above problems.
+	 *
+	 * Note that we capture the current size before entering the critical
+	 * section, so that we can use smgrtruncatefrom().  smgrtruncate() can't
+	 * be used in a critical section because it might allocate memory.
 	 */
+	START_CRIT_SECTION();
+
 	if (RelationNeedsWAL(rel))
 	{
 		/*
@@ -408,7 +425,9 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
 	 * longer exist after truncation is complete, and then truncate the
 	 * corresponding files on disk.
 	 */
-	smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks);
+	smgrtruncatefrom(RelationGetSmgr(rel), forks, nforks, old_blocks, 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 cc8a80ee961..1e509d42da5 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -1162,19 +1162,20 @@ mdnblocks(SMgrRelation reln, ForkNumber forknum)
 
 /*
  * mdtruncate() -- Truncate relation to specified number of blocks.
+ *
+ * Guaranteed not to allocate memory, so it can be used in a critical section.
+ * Caller must have called smgrnblocks() to obtain curnblk while holding a
+ * sufficient lock to prevent a change in relation size.  This also makes sure
+ * we have opened all active segments, so that truncate loop will get them
+ * all!  That step can't be done in a critical section.
  */
 void
-mdtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks)
+mdtruncate(SMgrRelation reln, ForkNumber forknum,
+		   BlockNumber curnblk, BlockNumber nblocks)
 {
-	BlockNumber curnblk;
 	BlockNumber priorblocks;
 	int			curopensegs;
 
-	/*
-	 * NOTE: mdnblocks makes sure we have opened all active segments, so that
-	 * truncation loop will get them all!
-	 */
-	curnblk = mdnblocks(reln, forknum);
 	if (nblocks > curnblk)
 	{
 		/* Bogus request ... but no complaint if InRecovery */
@@ -1513,7 +1514,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
@@ -1525,6 +1526,16 @@ _fdvec_resize(SMgrRelation reln,
 			repalloc(reln->md_seg_fds[forknum],
 					 sizeof(MdfdVec) * nseg);
 	}
+	else
+	{
+		/*
+		 * We don't reallocate a smaller array, because we want mdtruncate()
+		 * to be able to promise that it won't allocate memory, so that it is
+		 * allowed 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;
 }
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 925728eb6c1..0aba474134f 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -101,7 +101,7 @@ typedef struct f_smgr
 								   BlockNumber blocknum, BlockNumber nblocks);
 	BlockNumber (*smgr_nblocks) (SMgrRelation reln, ForkNumber forknum);
 	void		(*smgr_truncate) (SMgrRelation reln, ForkNumber forknum,
-								  BlockNumber nblocks);
+								  BlockNumber old_blocks, BlockNumber nblocks);
 	void		(*smgr_immedsync) (SMgrRelation reln, ForkNumber forknum);
 	void		(*smgr_registersync) (SMgrRelation reln, ForkNumber forknum);
 } f_smgr;
@@ -723,6 +723,32 @@ smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum)
  */
 void
 smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nblocks)
+{
+	BlockNumber old_nblocks[MAX_FORKNUM + 1];
+
+	/*
+	 * smgrnblocks() might need to allocate, but see smgrtruncatefrom() for a
+	 * version that can be used in a critical section.
+	 */
+	Assert(CritSectionCount == 0);
+
+	for (int i = 0; i < nforks; ++i)
+		old_nblocks[i] = smgrnblocks(reln, forknum[i]);
+
+	return smgrtruncatefrom(reln, forknum, nforks, old_nblocks, nblocks);
+}
+
+/*
+ * smgrtruncatefrom() -- Like sgmrtruncate(), but with caller-supplied old
+ *					 sizes, to allow usage in critical sections.
+ *
+ * See smgrtruncate() for requirements, and additionally the caller must call
+ * smgrnblocks() to obtain the size of each fork to be truncated, and not
+ * allow the relation to be invalidated in between.
+ */
+void
+smgrtruncatefrom(SMgrRelation reln, ForkNumber *forknum, int nforks,
+				 BlockNumber *old_nblocks, BlockNumber *nblocks)
 {
 	int			i;
 
@@ -750,7 +776,8 @@ smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nb
 		/* Make the cached size is invalid if we encounter an error. */
 		reln->smgr_cached_nblocks[forknum[i]] = InvalidBlockNumber;
 
-		smgrsw[reln->smgr_which].smgr_truncate(reln, forknum[i], nblocks[i]);
+		smgrsw[reln->smgr_which].smgr_truncate(reln, forknum[i],
+											   old_nblocks[i], nblocks[i]);
 
 		/*
 		 * We might as well update the local smgr_cached_nblocks values. The
diff --git a/src/include/storage/md.h b/src/include/storage/md.h
index b72293c79a5..e7671dd6c18 100644
--- a/src/include/storage/md.h
+++ b/src/include/storage/md.h
@@ -43,7 +43,7 @@ extern void mdwriteback(SMgrRelation reln, ForkNumber forknum,
 						BlockNumber blocknum, BlockNumber nblocks);
 extern BlockNumber mdnblocks(SMgrRelation reln, ForkNumber forknum);
 extern void mdtruncate(SMgrRelation reln, ForkNumber forknum,
-					   BlockNumber nblocks);
+					   BlockNumber old_blocks, BlockNumber nblocks);
 extern void mdimmedsync(SMgrRelation reln, ForkNumber forknum);
 extern void mdregistersync(SMgrRelation reln, ForkNumber forknum);
 
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 5ab992f5bd5..08c8b6dc289 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -107,6 +107,9 @@ extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum);
 extern BlockNumber smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum);
 extern void smgrtruncate(SMgrRelation reln, ForkNumber *forknum,
 						 int nforks, BlockNumber *nblocks);
+extern void smgrtruncatefrom(SMgrRelation reln, ForkNumber *forknum,
+							 int nforks,
+							 BlockNumber *old_nblocks, BlockNumber *nblocks);
 extern void smgrimmedsync(SMgrRelation reln, ForkNumber forknum);
 extern void smgrregistersync(SMgrRelation reln, ForkNumber forknum);
 extern void AtEOXact_SMgr(void);
-- 
2.39.5