Date: Wed, 19 Sep 2018 11:32:11 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Laurenz Albe <laurenz.albe@cybertec.at>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code
 for Wi
Message-ID: <20180919023211.GC1650@paquier.xyz>
References: <19655.1537195712@sss.pgh.pa.us>
 <f62917d5-cc77-c6ac-847f-44f922ccdd97@2ndQuadrant.com>
 <3883.1537200836@sss.pgh.pa.us>
 <11148.1537227504@sss.pgh.pa.us>
 <20180918001143.GG31460@paquier.xyz>
 <20180918024227.GI31460@paquier.xyz>
 <23296.1537281909@sss.pgh.pa.us>
 <71bf47219037a5e3839748eb55e4fa175b58f523.camel@cybertec.at>
 <25361.1537285137@sss.pgh.pa.us>
 <c68010480459c79f24a015096ae43b6f0bcd9854.camel@cybertec.at>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="at6+YcpfzWZg/htY"
Content-Disposition: inline
In-Reply-To: <c68010480459c79f24a015096ae43b6f0bcd9854.camel@cybertec.at>
Precedence: bulk


--at6+YcpfzWZg/htY
Content-Type: multipart/mixed; boundary="bKyqfOwhbdpXa4YI"
Content-Disposition: inline


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

On Tue, Sep 18, 2018 at 06:04:58PM +0200, Laurenz Albe wrote:
> That wouldn't influence pipes, which was what Michael said was a
> problem for pg_dump.

Yeah, the authentication blows up badly on that..  You can see all the
tests using user and database names with all ASCII range turning red.

> I currently have no Windows system close, so I cannot test...

I have spent a large portion of my morning trying to test all the
solutions proposed, and a winner shows up.  Attached are three patches
which present all the solutions mentioned, attached here for visibility:
- win32-open-michael.patch, or the solution I was working on.  This has
led me actually nowhere.  Even trying to enforce _fmode to happen only
on the frontend has caused breakages of pg_dump for authentication.
Trying to be smarter than what other binaries do is nice and consistent
with the Windows experience I think, still it looks that this can lead
to breakages when a utility uses setmode() for some of the output
files.  I noticed that pgwin32_open missed to enforce the mode used when
calling _fdmode, still that solves nothing.
- win32-open-tom.patch, which switches pgwin32_fopen() to use text mode
all the time if binary is not specified.  While this looked promising,
I have noticed that this has been causing the same set of errors as what
my previous patch has been doing in pg_dump TAP tests.  Anyway, a
solution needs also to happen for pgwin32_open() as we want direct calls
to it to get the right call.
- win32-open-laurenz.patch, which enforces to text mode only if binary
mode is not defined, which maps strictly to what pre-11 is doing when
calling the system _open or _fopen.  And surprisingly, this is proving
to pass all the tests I ran: bincheck (including pgbench and pg_dump),
upgradecheck, recoverycheck, check, etc.  initdb --pwfile is not
complaining to me either.

So the odds are that Laurenz's solution is what we are looking for.
Laurenz, Tom, any thoughts to share?  I won't back-patch that ;)
--
Michael

--bKyqfOwhbdpXa4YI
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="win32-open-michael.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index b53d6eb9cc..cb8c7450d9 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -491,15 +491,7 @@ readfile(const char *path)
 	char	   *buffer;
 	int			c;
=20
-#ifdef WIN32
-	/*
-	 * On Windows, we have to open the file in text mode so that carriage
-	 * returns are stripped.
-	 */
-	if ((infile =3D fopen(path, "rt")) =3D=3D NULL)
-#else
 	if ((infile =3D fopen(path, "r")) =3D=3D NULL)
-#endif
 	{
 		fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
 				progname, path, strerror(errno));
diff --git a/src/port/open.c b/src/port/open.c
index a3ad946a60..9174cdce66 100644
--- a/src/port/open.c
+++ b/src/port/open.c
@@ -122,11 +122,35 @@ pgwin32_open(const char *fileName, int fileFlags,...)
 		return -1;
 	}
=20
+	/*
+	 * If caller has set neither O_BINARY nor O_TEXT, then look for what
+	 * the default mode is for this process, then enforce it.  This can
+	 * be done only after opening the file and before switching the file
+	 * mode.
+	 */
+	if ((fileFlags & (O_BINARY | O_TEXT)) =3D=3D 0)
+	{
+		int		pmode =3D 0;
+
+		/* only MSVC newer than 2015 support _get_fmode */
+#if (_MSC_VER >=3D 1900)
+		if (_get_fmode(&pmode) < 0)
+		{
+			/* _get_fmode sets errno */
+			CloseHandle(h);
+			return -1;
+		}
+#else
+		pmode =3D _fmode;
+#endif
+
+		fileFlags |=3D pmode;
+	}
+
 	/* _open_osfhandle will, on error, set errno accordingly */
 	if ((fd =3D _open_osfhandle((intptr_t) h, fileFlags & O_APPEND)) < 0)
 		CloseHandle(h);			/* will not affect errno */
-	else if (fileFlags & (O_TEXT | O_BINARY) &&
-			 _setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0)
+	else if (_setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0)
 	{
 		_close(fd);
 		return -1;
@@ -138,6 +162,7 @@ pgwin32_open(const char *fileName, int fileFlags,...)
 FILE *
 pgwin32_fopen(const char *fileName, const char *mode)
 {
+	char		fdmode[32];
 	int			openmode =3D 0;
 	int			fd;
=20
@@ -160,7 +185,36 @@ pgwin32_fopen(const char *fileName, const char *mode)
 	fd =3D pgwin32_open(fileName, openmode);
 	if (fd =3D=3D -1)
 		return NULL;
-	return _fdopen(fd, mode);
+
+	strcpy(fdmode, mode);
+
+	/*
+	 * Like pgwin32_open, look for the default mode to be used for this
+	 * file descriptor.  Note that it is important to do that again here
+	 * for _fdopen below.
+	 */
+	if ((openmode & (O_BINARY | O_TEXT)) =3D=3D 0)
+	{
+		int		pmode =3D 0;
+
+		/* only MSVC newer than 2015 support _get_fmode */
+#if (_MSC_VER >=3D 1900)
+		if (_get_fmode(&pmode) < 0)
+		{
+			/* get_fmode sets errno */
+			_close(fd);
+			return NULL;
+		}
+#else
+		pmode =3D _fmode;
+#endif
+
+		snprintf(fdmode, sizeof(fdmode), "%s%s%s", fdmode,
+				 (pmode & O_BINARY) !=3D 0 ? "b" : "",
+				 (pmode & O_TEXT) !=3D 0 ? "t" : "");
+	}
+
+	return _fdopen(fd, fdmode);
 }
=20
 #endif

--bKyqfOwhbdpXa4YI
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="win32-open-laurenz.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index b53d6eb9cc..cb8c7450d9 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -491,15 +491,7 @@ readfile(const char *path)
 	char	   *buffer;
 	int			c;
=20
-#ifdef WIN32
-	/*
-	 * On Windows, we have to open the file in text mode so that carriage
-	 * returns are stripped.
-	 */
-	if ((infile =3D fopen(path, "rt")) =3D=3D NULL)
-#else
 	if ((infile =3D fopen(path, "r")) =3D=3D NULL)
-#endif
 	{
 		fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
 				progname, path, strerror(errno));
diff --git a/src/port/open.c b/src/port/open.c
index a3ad946a60..63a7a1d2fb 100644
--- a/src/port/open.c
+++ b/src/port/open.c
@@ -71,6 +71,19 @@ pgwin32_open(const char *fileName, int fileFlags,...)
 						 _O_SHORT_LIVED | O_DSYNC | O_DIRECT |
 						 (O_CREAT | O_TRUNC | O_EXCL) | (O_TEXT | O_BINARY))) =3D=3D fileFla=
gs);
=20
+#ifdef FRONTEND
+	/*
+	 * Since PostgreSQL 12, those concurrent-safe versions of open() and
+	 * fopen() can be used by frontends, having as side-effect to switch
+	 * the file-transaction mode from O_TEXT to O_BINARY if none is
+	 * specified.  Caller may want to enforce the binary or text mode,
+	 * but if nothing is defined make sure that the default mode maps
+	 * with what versions older than 12 have been doing.
+	 */
+	if ((fileFlags & O_BINARY) =3D=3D 0)
+		fileFlags |=3D O_TEXT;
+#endif
+
 	sa.nLength =3D sizeof(sa);
 	sa.bInheritHandle =3D TRUE;
 	sa.lpSecurityDescriptor =3D NULL;

--bKyqfOwhbdpXa4YI
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="win32-open-tom.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index b53d6eb9cc..cb8c7450d9 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -491,15 +491,7 @@ readfile(const char *path)
 	char	   *buffer;
 	int			c;
=20
-#ifdef WIN32
-	/*
-	 * On Windows, we have to open the file in text mode so that carriage
-	 * returns are stripped.
-	 */
-	if ((infile =3D fopen(path, "rt")) =3D=3D NULL)
-#else
 	if ((infile =3D fopen(path, "r")) =3D=3D NULL)
-#endif
 	{
 		fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
 				progname, path, strerror(errno));
diff --git a/src/port/open.c b/src/port/open.c
index a3ad946a60..85ab06e518 100644
--- a/src/port/open.c
+++ b/src/port/open.c
@@ -154,7 +154,7 @@ pgwin32_fopen(const char *fileName, const char *mode)
=20
 	if (strchr(mode, 'b'))
 		openmode |=3D O_BINARY;
-	if (strchr(mode, 't'))
+	else
 		openmode |=3D O_TEXT;
=20
 	fd =3D pgwin32_open(fileName, openmode);

--bKyqfOwhbdpXa4YI--

--at6+YcpfzWZg/htY
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAluhtSsACgkQnvQgOdby
QH1CxQ/+K+HaUn4RftM7oW1Phsie1iz0acYarq3/rW6HJcfVyu9b3mr1zj73kT8s
D09P9vyrX+gtmOKsX8ZmCpXf8Rtn7KqG+W7aapLtviAhlwBy4wvnmTpFvCStdiyF
0k6O1cxQrXSc4NLM6x4BbLibKX9zyzdEOxVFER7JfhwbnefDSVISG/zsHWYMCOoX
zpKLUFvNMpFylSe0r9Y/dxER8bPZbjzV9h/eDZbLwhSKzVLkhyYskAhE6J+/Rm3b
5DwJJopmy0NuNTWTMUlCNx2+1o6wVWCLZFIyH9wwrBonlDq53iILfZ6HsHSUxfoa
Jl8mFe6vSl6q2SI3t/vSS0n9xYEfMmCHqmubh6Jzu2K55QLZA1qnFR8XkmYeke8V
pw0/ZWLPyjqPdSmI0CDWoHfIElyeEnj7KpWyM/H9VeG6wf/gKFVvhW3CAqqiqP1t
ZR9TpPbuAwJExfvJO5bLBNJUZdY1/1qvL7WxnFRJLobAZdxytDU2Q/As5vatQqkG
HWpCB6raQfa/S4SzQQdkl3Y5XS8gO2UOcg0ZXQ+P0x4WQfBDuv4guzBSsHXpCcar
u8F122hyA5Nv/juOQsfgSNUKTfFeZ29L8Y3kYJvXSQpMShvMgFRlytp7SXuE6B+x
3oEgUcstog3wGliDbTURzPqWUl4Cxzqm5rIMkyDgV9udk8giHBc=
=2l7U
-----END PGP SIGNATURE-----

--at6+YcpfzWZg/htY--