From: Michael Paquier <michael@paquier.xyz>
To: Andres Freund <andres@anarazel.de>
Cc: buschmann@nidsa.net, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael.paquier@gmail.com>
Subject: Re: BUG #16039: PANIC when activating replication slots in Postgres 12.0 64bit under Windows
Date: Sun, 6 Oct 2019 13:55:48 +0900
Message-ID: <20191006045548.GA14532@paquier.xyz> (raw)
In-Reply-To: <20191004200605.yqcmn75otebwcvyj@alap3.anarazel.de>
References: <16039-196fc97cc05e141c@postgresql.org>
<20191004200605.yqcmn75otebwcvyj@alap3.anarazel.de>
On Fri, Oct 04, 2019 at 01:06:05PM -0700, Andres Freund wrote:
> I realize I perhaps should have added a comment explaining this, but
> this is far from the only location that knows we have to know open fds
> r/w to be able to fsync them.
Sorry for the late reply here. It looks like I messed up here, my
apologies for that. And thanks for fixing the issue.
It would have been nice to add some sanity checks based on fcntl() but
directory handling in pg_fsync() makes that annoying. Anyway, I have
checked the code with a little trick, and I have spotted a second bug:
CheckPointLogicalRewriteHeap() fsyncs a logical rewrite mapping file
with RDONLY. This is incorrect since b89e151.
> What were you even trying to fix by changing this?
Hardening of the code. Some code paths clearly relied on the
operations to be read-only.
> Seems also pretty clear that we need a few animals running with fsync
> enabled. Not sure how we best can write test infrastructure to make it
> easy to set that for all tests. Guess I best start a thread about it on
> -hackers.
I think that we would need more infrastructure here for TAP tests, aka
how to be able to enforce some parameters when setting the
configuration of a new node.
Attached are two patches: the actual bug fix and an extra patch with
the trick I have used to find it out (contrib/test_decoding/ was the
part which has blown up).
--
Michael
Attachments:download all (3 files, .tar.gz)fsync-chkpt-fix.patch (text/x-diff, 557B) downloaddiff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.cindex 7c98a42b8b..306e74ea10 100644--- a/src/backend/access/heap/rewriteheap.c+++ b/src/backend/access/heap/rewriteheap.c@@ -1280,7 +1280,8 @@ CheckPointLogicalRewriteHeap(void)
}
else
{
- int fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);+ /* on some operating systems fsyncing a file requires O_RDWR */+ int fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
/*
* The file cannot vanish due to concurrency since this function
fsync-trick.patch (text/x-diff, 1.3K) downloaddiff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.cindex 94be62fa6e..091f8dc902 100644--- a/src/backend/storage/file/fd.c+++ b/src/backend/storage/file/fd.c@@ -330,6 +330,9 @@ static int fsync_parent_path(const char *fname, int elevel);
int
pg_fsync(int fd)
{
+ /* on some operating systems fsyncing a file requires O_RDWR */+ Assert((fcntl(fd, F_GETFL) & (O_WRONLY | O_RDWR)) != 0);+
/* #if is to skip the sync_method test if there's no need for it */
#if defined(HAVE_FSYNC_WRITETHROUGH) && !defined(FSYNC_WRITETHROUGH_IS_FSYNC)
if (sync_method == SYNC_METHOD_FSYNC_WRITETHROUGH)
@@ -3358,6 +3361,10 @@ fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel)
else
flags |= O_RDONLY;
+ /* trick to bypass O_RDONLY check */+ if (isdir)+ return 0;+
fd = OpenTransientFile(fname, flags);
/*
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pmindex 270bd6c856..c727e8fa27 100644--- a/src/test/perl/PostgresNode.pm+++ b/src/test/perl/PostgresNode.pm@@ -441,7 +441,7 @@ sub init
open my $conf, '>>', "$pgdata/postgresql.conf";
print $conf "\n# Added by PostgresNode.pm\n";
- print $conf "fsync = off\n";+ print $conf "fsync = on\n";
print $conf "restart_after_crash = off\n";
print $conf "log_line_prefix = '%m [%p] %q%a '\n";
print $conf "log_statement = all\n";
signature.asc (application/pgp-signature, 832B) download
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-bugs@postgresql.org
Cc: michael@paquier.xyz, andres@anarazel.de, michael.paquier@gmail.com
Subject: Re: BUG #16039: PANIC when activating replication slots in Postgres 12.0 64bit under Windows
In-Reply-To: <20191006045548.GA14532@paquier.xyz>
* 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