public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
To: Robert Haas <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: [email protected]
Cc: Thomas Munro <[email protected]>
Cc: Melanie Plageman <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Subject: pg_basebackup fsyncs some files despite --no-sync (was: Adding CI to our tree)
Date: Fri, 21 Jan 2022 12:00:57 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<CA+Tgmoap6WXzta=zw97SF73hCfzEWVMtfDFB01svYXMtTzKVVA@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
Hi,
On 2022-01-18 20:16:46 -0800, Andres Freund wrote:
> I noticed a few other sources of "unnecessary" fsyncs. The most frequent
> being the durable_rename() of backup_manifest in pg_basebackup.c. Manifests are
> surprisingly large, 135k for a freshly initdb'd cluster.
Robert, I assume the fsync for manifests isn't ignoring --no-sync for a
particular reason?
The attached patch adds no-sync handling to the manifest rename, as well as
one case in the directory wal method.
It's a bit painful that we have to have code like
if (dir_data->sync)
r = durable_rename(tmppath, tmppath2);
else
{
if (rename(tmppath, tmppath2) != 0)
{
pg_log_error("could not rename file \"%s\" to \"%s\": %m",
tmppath, tmppath2);
r = -1;
}
}
It seems like it'd be better to set it up so that durable_rename() could
decide internally wether to fsync, or have a wrapper around durable_rename()?
> There's an fsync in walmethods.c:tar_close() that sounds intentional, but I
> don't really understand what the comment:
>
> /* Always fsync on close, so the padding gets fsynced */
> if (tar_sync(f) < 0)
tar_sync() actually checks for tar_data->sync, so it doesn't do an
fsync. Arguably the comment is a bit confusing, but ...
Greetings,
Andres Freund
Attachments:
[text/x-diff] v1-0001-pg_basebackup-Skip-a-few-more-fsyncs-if-no-sync-i.patch (2.1K, ../[email protected]/2-v1-0001-pg_basebackup-Skip-a-few-more-fsyncs-if-no-sync-i.patch)
download | inline diff:
From 36772524734f9f613edd6622d840e48f045f04d8 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Fri, 21 Jan 2022 10:30:37 -0800
Subject: [PATCH v1] pg_basebackup: Skip a few more fsyncs if --no-sync is
specified.
Author:
Reviewed-By:
Discussion: https://postgr.es/m/[email protected]
Backpatch:
---
src/bin/pg_basebackup/pg_basebackup.c | 18 +++++++++++++++---
src/bin/pg_basebackup/walmethods.c | 12 +++++++++++-
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index d5b0ade10d5..a5cca388845 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2196,9 +2196,21 @@ BaseBackup(void)
snprintf(tmp_filename, MAXPGPATH, "%s/backup_manifest.tmp", basedir);
snprintf(filename, MAXPGPATH, "%s/backup_manifest", basedir);
- /* durable_rename emits its own log message in case of failure */
- if (durable_rename(tmp_filename, filename) != 0)
- exit(1);
+ if (do_sync)
+ {
+ /* durable_rename emits its own log message in case of failure */
+ if (durable_rename(tmp_filename, filename) != 0)
+ exit(1);
+ }
+ else
+ {
+ if (rename(tmp_filename, filename) != 0)
+ {
+ pg_log_error("could not rename file \"%s\" to \"%s\": %m",
+ tmp_filename, filename);
+ exit(1);
+ }
+ }
}
if (verbose)
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index f74bd13315c..a6d08c1270a 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -445,7 +445,17 @@ dir_close(Walfile f, WalCloseMethod method)
snprintf(tmppath2, sizeof(tmppath2), "%s/%s",
dir_data->basedir, filename2);
pg_free(filename2);
- r = durable_rename(tmppath, tmppath2);
+ if (dir_data->sync)
+ r = durable_rename(tmppath, tmppath2);
+ else
+ {
+ if (rename(tmppath, tmppath2) != 0)
+ {
+ pg_log_error("could not rename file \"%s\" to \"%s\": %m",
+ tmppath, tmppath2);
+ r = -1;
+ }
+ }
}
else if (method == CLOSE_UNLINK)
{
--
2.34.0
view thread (22+ messages)
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], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: pg_basebackup fsyncs some files despite --no-sync (was: Adding CI to our tree)
In-Reply-To: <[email protected]>
* 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