From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Subject: Non-emergency patch for bug #17679
Date: Tue, 08 Nov 2022 11:28:08 -0500
Message-ID: <3797575.1667924888@sss.pgh.pa.us> (raw)
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) downloaddiff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.cindex 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))
{
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org, robertmhaas@gmail.com, andres@anarazel.de
Subject: Re: Non-emergency patch for bug #17679
In-Reply-To: <3797575.1667924888@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox