agora inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
Subject: [PATCH v5 1/2] Replace calls to durable_rename_excl() with durable_rename().
Date: Tue, 26 Apr 2022 11:56:50 -0700
durable_rename_excl() attempts to avoid overwriting any existing
files by using link() and unlink(), but it falls back to rename()
on some platforms (e.g., Windows), which offers no such overwrite
protection. Most callers use durable_rename_excl() just in case
there is an existing file, but in practice there shouldn't be one.
basic_archive used it to avoid overwriting an archive concurrently
created by another server, but as mentioned above, it will still
overwrite files on some platforms.
Furthermore, failures during durable_rename_excl() can result in
multiple hard links to the same file. My testing demonstrated that
it was possible to end up with two links to the same file in pg_wal
after a crash just before unlink() during WAL recycling.
Specifically, the test produced links to the same file for the
current WAL file and the next one because the half-recycled WAL
file was re-recycled upon restarting. This seems likely to lead to
WAL corruption.
This change replaces all calls to durable_rename_excl() with
durable_rename(). This removes the protection against
accidentally overwriting an existing file, but some platforms are
already living without it, and ordinarily there shouldn't be one.
The function itself is left around in case any extensions are using
it. It will be removed in v16 via a follow-up commit.
Back-patch to all supported versions. Before v13,
durable_rename_excl() was named durable_link_or_rename().
Author: Nathan Bossart
Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20220418182336.GA2298576%40nathanxps13
---
contrib/basic_archive/basic_archive.c | 5 +++--
src/backend/access/transam/timeline.c | 18 +++++-------------
src/backend/access/transam/xlog.c | 9 +++------
3 files changed, 11 insertions(+), 21 deletions(-)
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index e7efbfb9c3..ed33854c57 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -281,9 +281,10 @@ basic_archive_file_internal(const char *file, const char *path)
/*
* Sync the temporary file to disk and move it to its final destination.
- * This will fail if destination already exists.
+ * Note that this will overwrite any existing file, but this is only
+ * possible if someone else created the file since the stat() above.
*/
- (void) durable_rename_excl(temp, destination, ERROR);
+ (void) durable_rename(temp, destination, ERROR);
ereport(DEBUG1,
(errmsg("archived \"%s\" via basic_archive", file)));
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index be21968293..e0a2a8ea68 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -441,12 +441,8 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
* Now move the completed history file into place with its final name.
*/
TLHistoryFilePath(path, newTLI);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ Assert(access(path, F_OK) != 0 && errno == ENOENT);
+ durable_rename(tmppath, path, ERROR);
/* The history file can be archived immediately. */
if (XLogArchivingActive())
@@ -516,15 +512,11 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content, int size)
errmsg("could not close file \"%s\": %m", tmppath)));
/*
- * Now move the completed history file into place with its final name.
+ * Now move the completed history file into place with its final name,
+ * replacing any existing file with the same name.
*/
TLHistoryFilePath(path, tli);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ durable_rename(tmppath, path, ERROR);
}
/*
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 61cda56c6f..76ec80c950 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3323,14 +3323,11 @@ InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
}
}
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- if (durable_rename_excl(tmppath, path, LOG) != 0)
+ Assert(access(path, F_OK) != 0 && errno == ENOENT);
+ if (durable_rename(tmppath, path, LOG) != 0)
{
LWLockRelease(ControlFileLock);
- /* durable_rename_excl already emitted log message */
+ /* durable_rename already emitted log message */
return false;
}
--
2.25.1
--ZGiS0Q5IWpPtfppv
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v5-0002-Remove-durable_rename_excl.patch"
view thread (6+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected]
Subject: Re: [PATCH v5 1/2] Replace calls to durable_rename_excl() with durable_rename().
In-Reply-To: <no-message-id-1859613@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox