[Tom Lane <tgl@sss.pgh.pa.us>]

In the release team's discussion leading up to commit 0e758ae89,
Andres opined that what commit 4ab5dae94 had done to mdunlinkfork
was a mess, and I concur.  It invented an entirely new code path
through that function, and required two different behaviors from the
segment-deletion loop.  I think a very straight line can be drawn
between that extra complexity and the introduction of a nasty bug.
It's all unnecessary too, because AFAICS all we really need is to
apply the pre-existing behavior for temp tables and REDO mode
to binary-upgrade mode as well.

Hence, the attached reverts everything 4ab5dae94 did to this function,
and most of 0e758ae89 too, and instead makes IsBinaryUpgrade an
additional reason to take the immediate-unlink path.

Barring objections, I'll push this after the release freeze lifts.

			regards, tom lane

Attachments:

fix-mdunlinkfork-more-cleanly.patch (text/x-diff, 3.4K) download
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index b1a2cb575d..0f642df3c1 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -263,6 +263,12 @@ mdcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo)
  * The fact that temp rels and regular rels have different file naming
  * patterns provides additional safety.
  *
+ * We also don't do it while performing a binary upgrade.  There is no reuse
+ * hazard in that case, since after a crash or even a simple ERROR, the
+ * upgrade fails and the whole cluster must be recreated from scratch.
+ * Furthermore, it is important to remove the files from disk immediately,
+ * because we may be about to reuse the same relfilenumber.
+ *
  * All the above applies only to the relation's main fork; other forks can
  * just be removed immediately, since they are not needed to prevent the
  * relfilenumber from being recycled.  Also, we do not carefully
@@ -319,14 +325,15 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo)
 {
 	char	   *path;
 	int			ret;
-	BlockNumber segno = 0;
 
 	path = relpath(rlocator, forknum);
 
 	/*
-	 * Delete or truncate the first segment.
+	 * Truncate and then unlink the first segment, or just register a request
+	 * to unlink it later, as described in the comments for mdunlink().
 	 */
-	if (isRedo || forknum != MAIN_FORKNUM || RelFileLocatorBackendIsTemp(rlocator))
+	if (isRedo || IsBinaryUpgrade || forknum != MAIN_FORKNUM ||
+		RelFileLocatorBackendIsTemp(rlocator))
 	{
 		if (!RelFileLocatorBackendIsTemp(rlocator))
 		{
@@ -351,7 +358,6 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo)
 				ereport(WARNING,
 						(errcode_for_file_access(),
 						 errmsg("could not remove file \"%s\": %m", path)));
-			segno++;
 		}
 	}
 	else
@@ -359,42 +365,25 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo)
 		/* Prevent other backends' fds from holding on to the disk space */
 		ret = do_truncate(path);
 
-		/*
-		 * Except during a binary upgrade, register request to unlink first
-		 * segment later, rather than now.
-		 *
-		 * If we're performing a binary upgrade, the dangers described in the
-		 * header comments for mdunlink() do not exist, since after a crash or
-		 * even a simple ERROR, the upgrade fails and the whole new cluster
-		 * must be recreated from scratch. And, on the other hand, it is
-		 * important to remove the files from disk immediately, because we may
-		 * be about to reuse the same relfilenumber.
-		 */
-		if (!IsBinaryUpgrade)
-		{
-			register_unlink_segment(rlocator, forknum, 0 /* first seg */ );
-			segno++;
-		}
+		/* Register request to unlink first segment later */
+		register_unlink_segment(rlocator, forknum, 0 /* first seg */ );
 	}
 
 	/*
-	 * Delete any remaining segments (we might or might not have dealt with
-	 * the first one above).
+	 * Delete any additional segments.
 	 */
 	if (ret >= 0)
 	{
 		char	   *segpath = (char *) palloc(strlen(path) + 12);
+		BlockNumber segno;
 
 		/*
 		 * Note that because we loop until getting ENOENT, we will correctly
 		 * remove all inactive segments as well as active ones.
 		 */
-		for (;; segno++)
+		for (segno = 1;; segno++)
 		{
-			if (segno == 0)
-				strcpy(segpath, path);
-			else
-				sprintf(segpath, "%s.%u", path, segno);
+			sprintf(segpath, "%s.%u", path, segno);
 
 			if (!RelFileLocatorBackendIsTemp(rlocator))
 			{