pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedclient-side fsync() error handling
8+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2020-02-24 1420617b14 Change client-side fsync_fname() to report errors fatally
* client-side fsync() error handling
@ 2020-02-11 08:22 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 8+ messages in thread
From: Peter Eisentraut @ 2020-02-11 08:22 UTC (permalink / raw)
To: pgsql-hackers
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.c
index 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
* Re: client-side fsync() error handling
@ 2020-02-12 05:28 Michael Paquier <michael@paquier.xyz>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2020-02-12 05:28 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: pgsql-hackers
On Tue, Feb 11, 2020 at 09:22:54AM +0100, Peter Eisentraut wrote:
> 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.
>
> Thoughts?
Doing things as you do in your patch sounds fine to me for this part.
Now, don't we need to care about durable_rename() and make the
panic-like failure an optional choice? From what I can see, this
routine is used now in the backend for pg_basebackup to rename
temporary history files or partial WAL segments.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: client-side fsync() error handling
@ 2020-02-13 09:02 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 8+ messages in thread
From: Peter Eisentraut @ 2020-02-13 09:02 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers
On 2020-02-12 06:28, Michael Paquier wrote:
> Now, don't we need to care about durable_rename() and make the
> panic-like failure an optional choice? From what I can see, this
> routine is used now in the backend for pg_basebackup to rename
> temporary history files or partial WAL segments.
durable_rename() calls fsync_fname(), so it would be covered by this
change. The other file access calls in there can be handled by normal
error handling, I think. Is there any specific scenario you have in mind?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: client-side fsync() error handling
@ 2020-02-13 11:52 Michael Paquier <michael@paquier.xyz>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2020-02-13 11:52 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: pgsql-hackers
On Thu, Feb 13, 2020 at 10:02:31AM +0100, Peter Eisentraut wrote:
> On 2020-02-12 06:28, Michael Paquier wrote:
>> Now, don't we need to care about durable_rename() and make the
>> panic-like failure an optional choice? From what I can see, this
>> routine is used now in the backend for pg_basebackup to rename
>> temporary history files or partial WAL segments.
>
> durable_rename() calls fsync_fname(), so it would be covered by this change.
> The other file access calls in there can be handled by normal error
> handling, I think. Is there any specific scenario you have in mind?
The old file flush is handled by your patch, but not the new one if
it exists, and it seems to me that we should handle failures
consistently to reason easier about it, actually as the top of the
function says :)
Another point that we could consider is if fsync_fname() should have
an option to not trigger an immediate exit when facing a failure. The
backend has that option thanks to fsync_fname_ext() with its elevel
argument. Your choice to default to a failure is fine for most cases
because that's what we want. However, I am questioning if this change
would be surprising for some client applications or not, and if we
should have the option to choose one behavior or the other.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: client-side fsync() error handling
@ 2020-02-20 09:10 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 8+ messages in thread
From: Peter Eisentraut @ 2020-02-20 09:10 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers
On 2020-02-13 12:52, Michael Paquier wrote:
>> durable_rename() calls fsync_fname(), so it would be covered by this change.
>> The other file access calls in there can be handled by normal error
>> handling, I think. Is there any specific scenario you have in mind?
>
> The old file flush is handled by your patch, but not the new one if
> it exists, and it seems to me that we should handle failures
> consistently to reason easier about it, actually as the top of the
> function says :)
OK, added in new patch.
> Another point that we could consider is if fsync_fname() should have
> an option to not trigger an immediate exit when facing a failure. The
> backend has that option thanks to fsync_fname_ext() with its elevel
> argument. Your choice to default to a failure is fine for most cases
> because that's what we want. However, I am questioning if this change
> would be surprising for some client applications or not, and if we
> should have the option to choose one behavior or the other.
The option in the backend is between panicking and retrying. The old
behavior was to always retry but we have learned that that usually
doesn't work.
The frontends do neither right now, or at least the error handling is
very inconsistent and inscrutable. It would be possible in theory to
add a retry option, but that would be a very different patch, and given
what we have learned about fsync(), it probably wouldn't be widely useful.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v2-0001-Change-client-side-fsync_fname-to-report-errors-f.patch (text/plain, 2.4K) download
From 91bd9bee1de18fbb1842d0223b693cff21eb2ec6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 20 Feb 2020 09:52:55 +0100
Subject: [PATCH v2] 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() and durable_rename() 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.
Discussion: https://www.postgresql.org/message-id/flat/d239d1bd-aef0-ca7c-dc0a-da14bdcf0392%402ndquadrant.com
---
src/common/file_utils.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index 413fe4eeb1..7584c1f2fb 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);
@@ -364,9 +362,9 @@ durable_rename(const char *oldfile, const char *newfile)
{
if (fsync(fd) != 0)
{
- pg_log_error("could not fsync file \"%s\": %m", newfile);
+ pg_log_fatal("could not fsync file \"%s\": %m", newfile);
close(fd);
- return -1;
+ exit(EXIT_FAILURE);
}
close(fd);
}
--
2.25.0
* Re: client-side fsync() error handling
@ 2020-02-21 04:18 Michael Paquier <michael@paquier.xyz>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2020-02-21 04:18 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: pgsql-hackers
On Thu, Feb 20, 2020 at 10:10:11AM +0100, Peter Eisentraut wrote:
> OK, added in new patch.
Thanks, that looks good.
> The frontends do neither right now, or at least the error handling is very
> inconsistent and inscrutable. It would be possible in theory to add a retry
> option, but that would be a very different patch, and given what we have
> learned about fsync(), it probably wouldn't be widely useful.
Perhaps. Let's have this discussion later if there are drawbacks
about changing things the way your patch does. If we don't do that,
we'll never know about it either and this patch makes things safer.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: client-side fsync() error handling
@ 2020-02-24 16:03 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 8+ messages in thread
From: Peter Eisentraut @ 2020-02-24 16:03 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers
On 2020-02-21 05:18, Michael Paquier wrote:
> On Thu, Feb 20, 2020 at 10:10:11AM +0100, Peter Eisentraut wrote:
>> OK, added in new patch.
>
> Thanks, that looks good.
committed
>> The frontends do neither right now, or at least the error handling is very
>> inconsistent and inscrutable. It would be possible in theory to add a retry
>> option, but that would be a very different patch, and given what we have
>> learned about fsync(), it probably wouldn't be widely useful.
>
> Perhaps. Let's have this discussion later if there are drawbacks
> about changing things the way your patch does. If we don't do that,
> we'll never know about it either and this patch makes things safer.
yup
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: client-side fsync() error handling
@ 2020-02-25 02:33 Michael Paquier <michael@paquier.xyz>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 0 replies; 8+ messages in thread
From: Michael Paquier @ 2020-02-25 02:33 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: pgsql-hackers
On Mon, Feb 24, 2020 at 05:03:07PM +0100, Peter Eisentraut wrote:
> committed
Thanks!
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2020-02-25 02:33 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-02-11 08:22 client-side fsync() error handling Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2020-02-12 05:28 ` Michael Paquier <michael@paquier.xyz>
2020-02-13 09:02 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2020-02-13 11:52 ` Michael Paquier <michael@paquier.xyz>
2020-02-20 09:10 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2020-02-21 04:18 ` Michael Paquier <michael@paquier.xyz>
2020-02-24 16:03 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2020-02-25 02:33 ` Michael Paquier <michael@paquier.xyz>
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