From: Michael Paquier <michael@paquier.xyz>
To: Laurenz Albe <laurenz.albe@cybertec.at>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi
Date: Wed, 19 Sep 2018 11:32:11 +0900
Message-ID: <20180919023211.GC1650@paquier.xyz> (raw)
In-Reply-To: <c68010480459c79f24a015096ae43b6f0bcd9854.camel@cybertec.at>
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>
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
Attachments:
[text/x-diff] win32-open-michael.patch (2.7K, ../20180919023211.GC1650@paquier.xyz/2-win32-open-michael.patch)
download | inline diff:diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.cindex 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;
-#ifdef WIN32- /*- * On Windows, we have to open the file in text mode so that carriage- * returns are stripped.- */- if ((infile = fopen(path, "rt")) == NULL)-#else
if ((infile = fopen(path, "r")) == 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.cindex 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;
}
+ /*+ * 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)) == 0)+ {+ int pmode = 0;++ /* only MSVC newer than 2015 support _get_fmode */+#if (_MSC_VER >= 1900)+ if (_get_fmode(&pmode) < 0)+ {+ /* _get_fmode sets errno */+ CloseHandle(h);+ return -1;+ }+#else+ pmode = _fmode;+#endif++ fileFlags |= pmode;+ }+
/* _open_osfhandle will, on error, set errno accordingly */
if ((fd = _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 = 0;
int fd;
@@ -160,7 +185,36 @@ pgwin32_fopen(const char *fileName, const char *mode)
fd = pgwin32_open(fileName, openmode);
if (fd == -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)) == 0)+ {+ int pmode = 0;++ /* only MSVC newer than 2015 support _get_fmode */+#if (_MSC_VER >= 1900)+ if (_get_fmode(&pmode) < 0)+ {+ /* get_fmode sets errno */+ _close(fd);+ return NULL;+ }+#else+ pmode = _fmode;+#endif++ snprintf(fdmode, sizeof(fdmode), "%s%s%s", fdmode,+ (pmode & O_BINARY) != 0 ? "b" : "",+ (pmode & O_TEXT) != 0 ? "t" : "");+ }++ return _fdopen(fd, fdmode);
}
#endif
[text/x-diff] win32-open-laurenz.patch (1.5K, ../20180919023211.GC1650@paquier.xyz/3-win32-open-laurenz.patch)
download | inline diff:diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.cindex 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;
-#ifdef WIN32- /*- * On Windows, we have to open the file in text mode so that carriage- * returns are stripped.- */- if ((infile = fopen(path, "rt")) == NULL)-#else
if ((infile = fopen(path, "r")) == 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.cindex 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))) == fileFlags);
+#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) == 0)+ fileFlags |= O_TEXT;+#endif+
sa.nLength = sizeof(sa);
sa.bInheritHandle = TRUE;
sa.lpSecurityDescriptor = NULL;
[text/x-diff] win32-open-tom.patch (932B, ../20180919023211.GC1650@paquier.xyz/4-win32-open-tom.patch)
download | inline diff:diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.cindex 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;
-#ifdef WIN32- /*- * On Windows, we have to open the file in text mode so that carriage- * returns are stripped.- */- if ((infile = fopen(path, "rt")) == NULL)-#else
if ((infile = fopen(path, "r")) == 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.cindex 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)
if (strchr(mode, 'b'))
openmode |= O_BINARY;
- if (strchr(mode, 't'))+ else
openmode |= O_TEXT;
fd = pgwin32_open(fileName, openmode);
[application/pgp-signature] signature.asc (832B, ../20180919023211.GC1650@paquier.xyz/5-signature.asc)
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-committers@postgresql.org
Cc: michael@paquier.xyz, laurenz.albe@cybertec.at, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi
In-Reply-To: <20180919023211.GC1650@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