From: Michael Paquier <michael@paquier.xyz>
To: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Bug in slot.c and are replication slots ever used at Window?
Date: Fri, 31 Aug 2018 11:46:47 -0700
Message-ID: <20180831184647.GC5305@paquier.xyz> (raw)
In-Reply-To: <20180830182731.GE15446@paquier.xyz>
References: <9eb1a6d5-b66f-2640-598d-c5ea46b8f68a@postgrespro.ru>
<20180830182731.GE15446@paquier.xyz>
Hi Konstantin,
On Thu, Aug 30, 2018 at 11:27:31AM -0700, Michael Paquier wrote:
> It seems to me that you are right here, "path" points to
> pg_replslot/$SLOTNAME/state which is a file so the fsync is incorrect.
> I am not sure that we'd want to publish fsync_parent_path out of fd.c
> though, so perhaps we could just save the slot path in a different
> variable and use it?
I have spent more time on this bug, and the code path you have pointed
at is the only one having such an issue. Attached is a patch to fix the
problem. It includes the sanity checks I have used to check all code
paths calling fsync_fname() for both the frontend and the backend code.
The checks will not be included in the final fix, still they look useful
so I am planning to start a new thread on the matter as perhaps other
folks have more and/or better ideas.
--
Michael
Attachments:slotpath_sync.patch (text/x-diff, 2.4K) downloaddiff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.cindex 19978d9a9e..4f30904141 100644--- a/src/backend/replication/slot.c+++ b/src/backend/replication/slot.c@@ -1352,6 +1352,7 @@ RestoreSlotFromDisk(const char *name)
{
ReplicationSlotOnDisk cp;
int i;
+ char slotdir[MAXPGPATH];
char path[MAXPGPATH + 22];
int fd;
bool restored = false;
@@ -1361,13 +1362,14 @@ RestoreSlotFromDisk(const char *name)
/* no need to lock here, no concurrent access allowed yet */
/* delete temp file if it exists */
- sprintf(path, "pg_replslot/%s/state.tmp", name);+ sprintf(slotdir, "pg_replslot/%s", name);+ sprintf(path, "%s/state.tmp", slotdir);
if (unlink(path) < 0 && errno != ENOENT)
ereport(PANIC,
(errcode_for_file_access(),
errmsg("could not remove file \"%s\": %m", path)));
- sprintf(path, "pg_replslot/%s/state", name);+ sprintf(path, "%s/state", slotdir);
elog(DEBUG1, "restoring replication slot from \"%s\"", path);
@@ -1402,7 +1404,7 @@ RestoreSlotFromDisk(const char *name)
/* Also sync the parent directory */
START_CRIT_SECTION();
- fsync_fname(path, true);+ fsync_fname(slotdir, true);
END_CRIT_SECTION();
/* read part of statefile that's guaranteed to be version independent */
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.cindex 8dd51f1767..49a5640c61 100644--- a/src/backend/storage/file/fd.c+++ b/src/backend/storage/file/fd.c@@ -574,6 +574,14 @@ pg_flush_data(int fd, off_t offset, off_t nbytes)
void
fsync_fname(const char *fname, bool isdir)
{
+#ifdef USE_ASSERT_CHECKING+ struct stat statbuf;++ stat(fname, &statbuf);+ Assert((isdir && S_ISDIR(statbuf.st_mode)) ||+ (!isdir && !S_ISDIR(statbuf.st_mode)));+#endif+
fsync_fname_ext(fname, isdir, false, ERROR);
}
diff --git a/src/common/file_utils.c b/src/common/file_utils.cindex 48876061c3..36095b01af 100644--- a/src/common/file_utils.c+++ b/src/common/file_utils.c@@ -266,6 +266,14 @@ fsync_fname(const char *fname, bool isdir, const char *progname)
int flags;
int returncode;
+#ifdef USE_ASSERT_CHECKING+ struct stat statbuf;++ stat(fname, &statbuf);+ Assert((isdir && S_ISDIR(statbuf.st_mode)) ||+ (!isdir && !S_ISDIR(statbuf.st_mode)));+#endif+
/*
* Some OSs require directories to be opened read-only whereas other
* systems don't allow us to fsync files opened read-only; so we need both
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-hackers@postgresql.org
Cc: michael@paquier.xyz, k.knizhnik@postgrespro.ru
Subject: Re: Bug in slot.c and are replication slots ever used at Window?
In-Reply-To: <20180831184647.GC5305@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