Date: Sun, 6 Oct 2019 13:55:48 +0900
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
Message-ID: <20191006045548.GA14532@paquier.xyz>
References: <16039-196fc97cc05e141c@postgresql.org>
 <20191004200605.yqcmn75otebwcvyj@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ZoaI/ZTpAVc4A5k6"
Content-Disposition: inline
In-Reply-To: <20191004200605.yqcmn75otebwcvyj@alap3.anarazel.de>
Precedence: bulk


--ZoaI/ZTpAVc4A5k6
Content-Type: multipart/mixed; boundary="jI8keyz6grp/JLjh"
Content-Disposition: inline


--jI8keyz6grp/JLjh
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--jI8keyz6grp/JLjh
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="fsync-chkpt-fix.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/hea=
p/rewriteheap.c
index 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 =3D OpenTransientFile(path, O_RDONLY | PG_BINARY);
+			/* on some operating systems fsyncing a file requires O_RDWR */
+			int			fd =3D OpenTransientFile(path, O_RDWR | PG_BINARY);
=20
 			/*
 			 * The file cannot vanish due to concurrency since this function

--jI8keyz6grp/JLjh
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="fsync-trick.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 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 ele=
vel);
 int
 pg_fsync(int fd)
 {
+	/* on some operating systems fsyncing a file requires O_RDWR */
+	Assert((fcntl(fd, F_GETFL) & (O_WRONLY | O_RDWR)) !=3D 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_FSY=
NC)
 	if (sync_method =3D=3D SYNC_METHOD_FSYNC_WRITETHROUGH)
@@ -3358,6 +3361,10 @@ fsync_fname_ext(const char *fname, bool isdir, bool =
ignore_perm, int elevel)
 	else
 		flags |=3D O_RDONLY;
=20
+	/* trick to bypass O_RDONLY check */
+	if (isdir)
+		return 0;
+
 	fd =3D OpenTransientFile(fname, flags);
=20
 	/*
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 270bd6c856..c727e8fa27 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -441,7 +441,7 @@ sub init
=20
 	open my $conf, '>>', "$pgdata/postgresql.conf";
 	print $conf "\n# Added by PostgresNode.pm\n";
-	print $conf "fsync =3D off\n";
+	print $conf "fsync =3D on\n";
 	print $conf "restart_after_crash =3D off\n";
 	print $conf "log_line_prefix =3D '%m [%p] %q%a '\n";
 	print $conf "log_statement =3D all\n";

--jI8keyz6grp/JLjh--

--ZoaI/ZTpAVc4A5k6
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl2Zc9QACgkQnvQgOdby
QH0FDw/8CBTwP/5LwF0JAuix22kQ7h9gsPdXflYJS5jVbCHfgSA77apsKeS9PLc1
1fG9cGocnX6DVcS7EkxSs28P6Ck+QG5IoMshdHMERh/O717QFCNjon0qOn0ZXJDH
p9rKxtMuGimioy1GhVp0iyVuXAViSIWmgVaUzrxX5SNAAQ4ZiNLNvpd5Df0iOP9Z
iom0s18LjMSFhs0ylJ9l8rDGAx9wLJ4YKdLXEx8/mBh+DzJoL6uCNXoHXofLhRQE
ufxebSydf/EE01yIKnJlsGfDItW92MsHsxPCIyYzLQTG2JF7C3GZHQrPW6VvmaNX
7JxMZ52/YGjBw9ZES+OGiYbQfKTqkgjFEzyco/+VKUk4y7k3T4mxxDyK/MYltevT
mGpG/QDR1M0lDrzZAO099pZuiBK7lSHJkB4TG0AsEPGc9mLbVdY8BKQR8djgXgbg
Arx69ztNXyKfvbgrBWrLctyRoCBRETrjXAXnQwFDtSyiVV3P2WjlAwKUZjXJSfAR
nLaH2TsH09fOy3D7JtaDcWNxELxB1593qkHcF4KbZefORZhNTxqFjd2cwwhup9Ao
Uf9V3QuwOjrbsr3UhFvY5fk1Ul1jkgRb3tlaPHCWKcyfYpWGklQEma2i7ARhKvu7
XmbPzvlK6l69m6GYmPxwh9s1csU9fpbLz5YGQiCVWuC/ZcVsXBY=
=RVS1
-----END PGP SIGNATURE-----

--ZoaI/ZTpAVc4A5k6--