From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: client-side fsync() error handling
Date: Tue, 11 Feb 2020 09:22:54 +0100
Message-ID: <d239d1bd-aef0-ca7c-dc0a-da14bdcf0392@2ndquadrant.com> (raw)
Continuing the discussion from [0], there are still a number of fsync()
calls in client programs that are unchecked or where errors are treated
non-fatally.
Digging around through the call stack, I think changing fsync_fname() to
just call exit(1) on errors instead of proceeding would address most of
that.
This affects (at least) initdb, pg_basebackup, pg_checksums, pg_dump,
pg_dumpall, and pg_rewind.
Thoughts?
[0]:
https://www.postgresql.org/message-id/flat/9b49fe44-8f3e-eca9-5914-29e9e99030bf%402ndquadrant.com
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:0001-Change-client-side-fsync_fname-to-report-errors-fata.patch (text/plain, 2.0K) download
From dfe2140d34d3c50b2908556c4627d77d009e487f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 11 Feb 2020 09:07:59 +0100
Subject: [PATCH] Change client-side fsync_fname() to report errors fatally
Given all we have learned about fsync() error handling in the last few
years, reporting an fsync() error non-fatally is not useful,
unless you don't care much about the file, in which case you probably
don't need to use fsync() in the first place.
Change fsync_fname() to exit(1) on fsync() errors other than those
that we specifically chose to ignore.
This affects initdb, pg_basebackup, pg_checksums, pg_dump, pg_dumpall,
and pg_rewind.
---
src/common/file_utils.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/common/file_utils.c b/src/common/file_utils.cindex 413fe4eeb1..078980a954 100644--- a/src/common/file_utils.c+++ b/src/common/file_utils.c@@ -51,8 +51,6 @@ static void walkdir(const char *path,
* fsyncing, and might not have privileges to write at all.
*
* serverVersion indicates the version of the server to be fsync'd.
- *- * Errors are reported but not considered fatal.
*/
void
fsync_pgdata(const char *pg_data,
@@ -250,8 +248,8 @@ pre_sync_fname(const char *fname, bool isdir)
* fsync_fname -- Try to fsync a file or directory
*
* Ignores errors trying to open unreadable files, or trying to fsync
- * directories on systems where that isn't allowed/required. Reports- * other errors non-fatally.+ * directories on systems where that isn't allowed/required. All other errors+ * are fatal.
*/
int
fsync_fname(const char *fname, bool isdir)
@@ -294,9 +292,9 @@ fsync_fname(const char *fname, bool isdir)
*/
if (returncode != 0 && !(isdir && (errno == EBADF || errno == EINVAL)))
{
- pg_log_error("could not fsync file \"%s\": %m", fname);+ pg_log_fatal("could not fsync file \"%s\": %m", fname);
(void) close(fd);
- return -1;+ exit(EXIT_FAILURE);
}
(void) close(fd);
--
2.25.0
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: peter.eisentraut@2ndquadrant.com
Subject: Re: client-side fsync() error handling
In-Reply-To: <d239d1bd-aef0-ca7c-dc0a-da14bdcf0392@2ndquadrant.com>
* 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