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
>From 36772524734f9f613edd6622d840e48f045f04d8 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
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/20220119041646.rhuo3youiqxqj...@alap3.anarazel.de
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
