Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Thu, Jul 23, 2020 at 01:05:04PM -0400, Jeff Janes wrote:
> I have noticed this before, but since it wasn't a production machine I just
> shrugged it off as being a hazard of using consumer-grade stuff; it didn't
> seem to be worth investigating further.

The most direct and non-invasive way to address this problem in
back-branches would be to copy the non-concurrent logic in
src/port/open.c directly into pg_test_fsync.c.  We have done that in
the past such things for example with restricted tokens on Windows for
pg_upgrade, see fa1e5afa but that was much more important.  I am not
sure that if it is worth the time spent here though, as it took
roughly 10 years to notice the problem (and I don't really count the
time where the tool was under src/tools/ but this did not build the
tool on Windows).
--
Michael


signature.asc
Description: PGP signature

Re: Loaded footgun open_datasync on Windows

[Jeff Janes]

On Fri, Sep 14, 2018 at 3:32 AM Michael Paquier  wrote:

> On Fri, Sep 14, 2018 at 08:43:18AM +0200, Laurenz Albe wrote:
>
> > If it turns out not to break anything, would you consider backpatching?
> > On the one hand it fixes a bug, on the other hand it affects all
> > frontend executables...
>
> Yeah, for this reason I would not do a backpatch.  I have a very hard
> time to believe that any frontend tools on Windows developed by anybody
> rely on files to be opened only by a single process, still if they do
> they would be surprised to see a change of behavior after a minor
> update in case they rely on the concurrency limitations.
>

Reviving an old thread here.

Could it be back-patched in some pg_test_fsync specific variant?  I
don't think we should just ignore the fact that pg_test_fsync on Windows is
unfit for its intended purpose on 4 still-supported versions.


> > I wonder why nobody noticed the problem in pg_test_fsync earlier.
> > Is it that people running Windows care less if their storage is
> > reliable?
>
> likely so.
>

I have noticed this before, but since it wasn't a production machine I just
shrugged it off as being a hazard of using consumer-grade stuff; it didn't
seem to be worth investigating further.

 Cheers,

Jeff

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Fri, Sep 14, 2018 at 08:43:18AM +0200, Laurenz Albe wrote:
> Thanks for being interested and doing the work.

No problem.  I have a sort of Windows-label stuck on me for ages, and
those random buildfarm failures are annoying with TAP tests on Windows.

> If it turns out not to break anything, would you consider backpatching?
> On the one hand it fixes a bug, on the other hand it affects all
> frontend executables...

Yeah, for this reason I would not do a backpatch.  I have a very hard
time to believe that any frontend tools on Windows developed by anybody
rely on files to be opened only by a single process, still if they do
they would be surprised to see a change of behavior after a minor
update in case they rely on the concurrency limitations.

> I wonder why nobody noticed the problem in pg_test_fsync earlier.
> Is it that people running Windows care less if their storage is
> reliable?

likely so.
--
Michael


signature.asc
Description: PGP signature

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

Michael Paquier wrote:
> On Thu, Sep 13, 2018 at 03:55:20PM +0200, Laurenz Albe wrote:
> > Attached is the new, improved version of the patch.
> 
> Thanks, finally pushed.  I have made sure that recovery TAP tests,
> upgrade tests and bin/ tests are working properly.

Thanks for being interested and doing the work.

If it turns out not to break anything, would you consider backpatching?
On the one hand it fixes a bug, on the other hand it affects all
frontend executables...

I wonder why nobody noticed the problem in pg_test_fsync earlier.
Is it that people running Windows care less if their storage is reliable?

Yours,
Laurenz Albe

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Thu, Sep 13, 2018 at 03:55:20PM +0200, Laurenz Albe wrote:
> Attached is the new, improved version of the patch.

Thanks, finally pushed.  I have made sure that recovery TAP tests,
upgrade tests and bin/ tests are working properly.
--
Michael


signature.asc
Description: PGP signature

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

Michael Paquier wrote:
> On Mon, Sep 10, 2018 at 04:46:40PM +0200, Laurenz Albe wrote:
> > I didn't get pg_upgrade to work without the log file hacks; I suspect
> > that there is more than just log file locking going on, but my Windows
> > skills are limited.
> > 
> > How shall I proceed?
> 
> I do like this patch, and we have an occasion to clean a bunch of things
> in pg_upgrade, so this argument is enough to me to put my hands in the
> dirt and check by myself, so...
> 
> I really thought that this was not ambitious enough, so I have hacked on
> top of your patch, so as pg_upgrade concurrent issues are removed, and I
> have found one barrier in pg_ctl which decides that it is smarter to
> redirect the log file (here pg_upgrade_server.log) using CMD.  The
> problem is that the lock taken by the process which does the redirection
> does not work nicely with what pg_upgrade does in parallel.  So I think
> that it is better to drop that part.

I got you now.
So I won't try to deal with that at this point.

> +#ifdef WIN32
> +   if ((infile = fopen(path, "rt")) == NULL)
> +#else
> if ((infile = fopen(path, "r")) == NULL)
> +#endif
> This should have a comment, saying roughly that as this uses
> win32_fopen, text mode needs to be enforced to get proper CRLF.

Done.

> One spot for open() is missed in file_utils.c, please see
> pre_sync_fname().

Done.

> The patch fails to apply for pg_verify_checksums, with a conflict easy
> enough to fix.

Fixed.

> Laurenz, could you update your patch?  I am switching that as waiting on
> author for now.

Attached is the new, improved version of the patch.

Thanks again!

Yours,
Laurenz Albe

From 8cd3d1a75fc3b18e6928b9b26841f1947d1f72ba Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Thu, 13 Sep 2018 14:24:59 +0200
Subject: [PATCH] Use pgwin32_open in frontend code on Windows

This is particularly important for pg_test_fsync which does not handle
O_DSYNC correctly otherwise, leading to false claims that disks are unsafe.

With pgwin32_open, files won't get locked when opened.
This could be used to get rid of some of the workarounds for Windows'
file locking, but that is left for later because it proved not to be as
trivial as hoped.

Discussion: https://www.postgresql.org/message-id/1527846213.2475.31.camel%40cybertec.at

Laurenz Albe, reviewed by Michael Paquier and Kuntal Ghosh
---
 src/bin/initdb/initdb.c   | 8 
 src/bin/pg_basebackup/pg_receivewal.c | 2 +-
 src/bin/pg_verify_checksums/pg_verify_checksums.c | 2 +-
 src/common/file_utils.c   | 4 ++--
 src/include/port.h| 3 ---
 5 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 32746c7703..bf234c0763 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -490,7 +490,15 @@ readfile(const char *path)
 	char	   *buffer;
 	int			c;
 
+#ifdef WIN32
+	/*
+	 * On Windows, we have to open the file in text mode
+	 * so that "carriage return" characters 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/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 8be8d48a8a..912aed8d7c 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -288,7 +288,7 @@ FindStreamingStart(uint32 *tli)
 
 			snprintf(fullpath, sizeof(fullpath), "%s/%s", basedir, dirent->d_name);
 
-			fd = open(fullpath, O_RDONLY | PG_BINARY);
+			fd = open(fullpath, O_RDONLY | PG_BINARY, 0);
 			if (fd < 0)
 			{
 fprintf(stderr, _("%s: could not open compressed file \"%s\": %s\n"),
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index d46bac2cd6..507f83ca4c 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -80,7 +80,7 @@ scan_file(const char *fn, BlockNumber segmentno)
 	int			f;
 	BlockNumber blockno;
 
-	f = open(fn, O_RDONLY | PG_BINARY);
+	f = open(fn, O_RDONLY | PG_BINARY, 0);
 	if (f < 0)
 	{
 		fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index 48876061c3..d952bc8c88 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -222,7 +222,7 @@ pre_sync_fname(const char *fname, bool isdir, const char *progname)
 {
 	int			fd;
 
-	fd = open(fname, O_RDONLY | PG_BINARY);
+	fd = open(fname, O_RDONLY | PG_BINARY, 0);
 
 	if (fd < 0)
 	{
@@ -283,7 +283,7 @@ fsync_fname(const char *fname, bool isdir, const char *progname)
 	 * unsupported operations, e.g. opening a directory under Windows), and
 	 * logging others.
 	 */
-	fd = open(fname, flags);
+	fd = open(fname, flags, 0);
 	if 

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

On Wed, 2018-09-12 at 14:43 +0900, Michael Paquier wrote:
> On Mon, Sep 10, 2018 at 04:46:40PM +0200, Laurenz Albe wrote:
> > I didn't get pg_upgrade to work without the log file hacks; I suspect
> > that there is more than just log file locking going on, but my Windows
> > skills are limited.
> > 
> > How shall I proceed?
> 
> I do like this patch, and we have an occasion to clean a bunch of things
> in pg_upgrade, so this argument is enough to me to put my hands in the
> dirt and check by myself, so...

Thanks for that and the rest of your review.

> > I have attached a new version, the previous one was bit-rotted.
> 
> I really thought that this was not ambitious enough, so I have hacked on
> top of your patch, so as pg_upgrade concurrent issues are removed, and I
> have found one barrier in pg_ctl which decides that it is smarter to
> redirect the log file (here pg_upgrade_server.log) using CMD.  The
> problem is that the lock taken by the process which does the redirection
> does not work nicely with what pg_upgrade does in parallel.  So I think
> that it is better to drop that part.

As soon as I get to our Windows machine with a bit of time on my hands,
I'll try to remove that hack in pg_ctl and see if that makes pg_upgrade
work without the WIN32 workarounds.

> +#ifdef WIN32
> +   if ((infile = fopen(path, "rt")) == NULL)
> +#else
> if ((infile = fopen(path, "r")) == NULL)
> +#endif
> This should have a comment, saying roughly that as this uses
> win32_fopen, text mode needs to be enforced to get proper CRLF.

Agreed, will do.

> One spot for open() is missed in file_utils.c, please see
> pre_sync_fname().

I missed that since PG_FLUSH_DATA_WORKS is never defined on Windows,
but I agree that it can't hurt to use the three-argument form of
open(2) there too, just in case Windows becomes more POSIX-compliant...

> The patch fails to apply for pg_verify_checksums, with a conflict easy
> enough to fix.

That's the bitrot I mentioned above; looks like I attached the wrong
version of the patch.  Will amend.

> At the end I would be incline to accept the patch proposed, knowing that
> this would fix https://postgr.es/m/16922.1520722...@sss.pgh.pa.us
> mentioned by Thomas upthread as get_pgpid would do things in a shared
> manner, putting an end at some of the random failures we've seen on the
> buildfarm.
> 
> Laurenz, could you update your patch?  I am switching that as waiting on
> author for now.

Thanks again!

Yours,
Laurenz Albe

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Mon, Sep 10, 2018 at 04:46:40PM +0200, Laurenz Albe wrote:
> I didn't get pg_upgrade to work without the log file hacks; I suspect
> that there is more than just log file locking going on, but my Windows
> skills are limited.
> 
> How shall I proceed?

I do like this patch, and we have an occasion to clean a bunch of things
in pg_upgrade, so this argument is enough to me to put my hands in the
dirt and check by myself, so...

> I think that it is important to get pg_test_fsync to work correctly on
> Windows, and if my patch does not break the buildfarm, that's what it
> does.

This argument argument is sound, still... [ ... suspense ... ]

> I have attached a new version, the previous one was bit-rotted.

I really thought that this was not ambitious enough, so I have hacked on
top of your patch, so as pg_upgrade concurrent issues are removed, and I
have found one barrier in pg_ctl which decides that it is smarter to
redirect the log file (here pg_upgrade_server.log) using CMD.  The
problem is that the lock taken by the process which does the redirection
does not work nicely with what pg_upgrade does in parallel.  So I think
that it is better to drop that part.

+#ifdef WIN32
+   if ((infile = fopen(path, "rt")) == NULL)
+#else
if ((infile = fopen(path, "r")) == NULL)
+#endif
This should have a comment, saying roughly that as this uses
win32_fopen, text mode needs to be enforced to get proper CRLF.

One spot for open() is missed in file_utils.c, please see
pre_sync_fname().

The patch fails to apply for pg_verify_checksums, with a conflict easy
enough to fix.

At the end I would be incline to accept the patch proposed, knowing that
this would fix https://postgr.es/m/16922.1520722...@sss.pgh.pa.us
mentioned by Thomas upthread as get_pgpid would do things in a shared
manner, putting an end at some of the random failures we've seen on the
buildfarm.

Laurenz, could you update your patch?  I am switching that as waiting on
author for now.
Thanks,
--
Michael


signature.asc
Description: PGP signature

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

Noah Misch wrote:
> On Wed, Jun 27, 2018 at 12:09:24PM +0200, Laurenz Albe wrote:
> > Michael Paquier wrote:
> > > > I have added it to the July commitfest.
> > > 
> > > Have you looked at the possibility of removing the log file constraints
> > > in pg_upgrade with the change you are doing here so as things would be
> > > more consistent with non-Windows platforms, simplifying some code on the
> > > way?
> > 
> > Can you explain what the "log file constraints" are?
> > If it is in any way related, and I can handle it, sure.
> 
> See this comment in pg_upgrade.h:
> 
> /*
>  * WIN32 files do not accept writes from multiple processes
>  *
>  * On Win32, we can't send both pg_upgrade output and command output to the
>  * same file because we get the error: "The process cannot access the file
>  * because it is being used by another process." so send the pg_ctl
>  * command-line output to a new file, rather than into the server log file.
>  * Ideally we could use UTILITY_LOG_FILE for this, but some Windows platforms
>  * keep the pg_ctl output file open by the running postmaster, even after
>  * pg_ctl exits.
>  *
>  * We could use the Windows pgwin32_open() flags to allow shared file
>  * writes but is unclear how all other tools would use those flags, so
>  * we just avoid it and log a little differently on Windows;  we adjust
>  * the error message appropriately.
>  */
> 
> If you grep src/bin/pg_upgrade for WIN32, roughly a third of the hits are
> workarounds for this problem.  I agree with Michael that removing those
> workarounds would be a good test of frontend pgwin32_open() and worth
> including in the initial commit.

Thank you for the explanation.

I didn't get pg_upgrade to work without the log file hacks; I suspect that there
is more than just log file locking going on, but my Windows skills are limited.

How shall I proceed?
I think that it is important to get pg_test_fsync to work correctly on Windows,
and if my patch does not break the buildfarm, that's what it does.

I have attached a new version, the previous one was bit-rotted.

Yours,
Laurenz Albe
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3f203c6ca6..9f6a9dc3d9 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -490,7 +490,11 @@ readfile(const char *path)
 	char	   *buffer;
 	int			c;
 
+#ifdef WIN32
+	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/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 8be8d48a8a..912aed8d7c 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -288,7 +288,7 @@ FindStreamingStart(uint32 *tli)
 
 			snprintf(fullpath, sizeof(fullpath), "%s/%s", basedir, dirent->d_name);
 
-			fd = open(fullpath, O_RDONLY | PG_BINARY);
+			fd = open(fullpath, O_RDONLY | PG_BINARY, 0);
 			if (fd < 0)
 			{
 fprintf(stderr, _("%s: could not open compressed file \"%s\": %s\n"),
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 28c975446e..d1a7121a26 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -83,7 +83,7 @@ scan_file(char *fn, int segmentno)
 	int			f;
 	int			blockno;
 
-	f = open(fn, 0);
+	f = open(fn, 0, 0);
 	if (f < 0)
 	{
 		fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index 48876061c3..8a4cdb47ff 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -283,7 +283,7 @@ fsync_fname(const char *fname, bool isdir, const char *progname)
 	 * unsupported operations, e.g. opening a directory under Windows), and
 	 * logging others.
 	 */
-	fd = open(fname, flags);
+	fd = open(fname, flags, 0);
 	if (fd < 0)
 	{
 		if (errno == EACCES || (isdir && errno == EISDIR))
diff --git a/src/include/port.h b/src/include/port.h
index 74a9dc4d4d..2d40045ad5 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -249,11 +249,8 @@ extern bool rmtree(const char *path, bool rmtopdir);
 #define		O_DIRECT	0x8000
 extern int	pgwin32_open(const char *, int,...);
 extern FILE *pgwin32_fopen(const char *, const char *);
-
-#ifndef FRONTEND
 #define		open(a,b,c) pgwin32_open(a,b,c)
 #define		fopen(a,b) pgwin32_fopen(a,b)
-#endif
 
 /*
  * Mingw-w64 headers #define popen and pclose to _popen and _pclose.  We want

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Sat, Sep 01, 2018 at 05:07:21PM -0700, Noah Misch wrote:
> If you grep src/bin/pg_upgrade for WIN32, roughly a third of the hits are
> workarounds for this problem.  I agree with Michael that removing those
> workarounds would be a good test of frontend pgwin32_open() and worth
> including in the initial commit.

Thanks Noah for jumping into the ship and answering the question from
Laurenz.

Laurenz, my apologies for not answering your previous question.  It
seems that I completely forgot about it.
--
Michael


signature.asc
Description: PGP signature

Re: Loaded footgun open_datasync on Windows

[Noah Misch]

On Wed, Jun 27, 2018 at 12:09:24PM +0200, Laurenz Albe wrote:
> Michael Paquier wrote:
> > > I have added it to the July commitfest.
> > 
> > Have you looked at the possibility of removing the log file constraints
> > in pg_upgrade with the change you are doing here so as things would be
> > more consistent with non-Windows platforms, simplifying some code on the
> > way?
> 
> Can you explain what the "log file constraints" are?
> If it is in any way related, and I can handle it, sure.

See this comment in pg_upgrade.h:

/*
 * WIN32 files do not accept writes from multiple processes
 *
 * On Win32, we can't send both pg_upgrade output and command output to the
 * same file because we get the error: "The process cannot access the file
 * because it is being used by another process." so send the pg_ctl
 * command-line output to a new file, rather than into the server log file.
 * Ideally we could use UTILITY_LOG_FILE for this, but some Windows platforms
 * keep the pg_ctl output file open by the running postmaster, even after
 * pg_ctl exits.
 *
 * We could use the Windows pgwin32_open() flags to allow shared file
 * writes but is unclear how all other tools would use those flags, so
 * we just avoid it and log a little differently on Windows;  we adjust
 * the error message appropriately.
 */

If you grep src/bin/pg_upgrade for WIN32, roughly a third of the hits are
workarounds for this problem.  I agree with Michael that removing those
workarounds would be a good test of frontend pgwin32_open() and worth
including in the initial commit.

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

Thomas Munro wrote:
> It looks like initdb is failing with this patch:
> 
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.6732
> 
> Unfortunately cfbot is not clever enough to print out the contents of
> the initdb log so I don't have any more information...

Sorry for the delay, I haven't been near a Windows machine lately.

Thanks for poking me, there was indeed a problem:
Now you have to tell initdb to open the PKI file as text file.

Attached is an updated patch.

Yours,
Laurenz Albediff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3f203c6ca6..9f6a9dc3d9 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -490,7 +490,11 @@ readfile(const char *path)
 	char	   *buffer;
 	int			c;
 
+#ifdef WIN32
+	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/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 8be8d48a8a..912aed8d7c 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -288,7 +288,7 @@ FindStreamingStart(uint32 *tli)
 
 			snprintf(fullpath, sizeof(fullpath), "%s/%s", basedir, dirent->d_name);
 
-			fd = open(fullpath, O_RDONLY | PG_BINARY);
+			fd = open(fullpath, O_RDONLY | PG_BINARY, 0);
 			if (fd < 0)
 			{
 fprintf(stderr, _("%s: could not open compressed file \"%s\": %s\n"),
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 28c975446e..d1a7121a26 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -83,7 +83,7 @@ scan_file(char *fn, int segmentno)
 	int			f;
 	int			blockno;
 
-	f = open(fn, 0);
+	f = open(fn, 0, 0);
 	if (f < 0)
 	{
 		fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index 48876061c3..8a4cdb47ff 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -283,7 +283,7 @@ fsync_fname(const char *fname, bool isdir, const char *progname)
 	 * unsupported operations, e.g. opening a directory under Windows), and
 	 * logging others.
 	 */
-	fd = open(fname, flags);
+	fd = open(fname, flags, 0);
 	if (fd < 0)
 	{
 		if (errno == EACCES || (isdir && errno == EISDIR))
diff --git a/src/include/port.h b/src/include/port.h
index 74a9dc4d4d..2d40045ad5 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -249,11 +249,8 @@ extern bool rmtree(const char *path, bool rmtopdir);
 #define		O_DIRECT	0x8000
 extern int	pgwin32_open(const char *, int,...);
 extern FILE *pgwin32_fopen(const char *, const char *);
-
-#ifndef FRONTEND
 #define		open(a,b,c) pgwin32_open(a,b,c)
 #define		fopen(a,b) pgwin32_fopen(a,b)
-#endif
 
 /*
  * Mingw-w64 headers #define popen and pclose to _popen and _pclose.  We want

Re: Loaded footgun open_datasync on Windows

[Thomas Munro]

On Thu, Jun 21, 2018 at 2:06 AM, Laurenz Albe  wrote:
> How about checking what the buildfarm thinks about the attached?

Hi Laurenz,

It looks like initdb is failing with this patch:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.6732

Unfortunately cfbot is not clever enough to print out the contents of
the initdb log so I don't have any more information...

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

Michael Paquier wrote:
> > I have added it to the July commitfest.
> 
> Have you looked at the possibility of removing the log file constraints
> in pg_upgrade with the change you are doing here so as things would be
> more consistent with non-Windows platforms, simplifying some code on the
> way?

Can you explain what the "log file constraints" are?
If it is in any way related, and I can handle it, sure.

Yours,
Laurenz Albe

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Mon, Jun 25, 2018 at 06:10:21PM +0200, Laurenz Albe wrote:
> I have added it to the July commitfest.

Have you looked at the possibility of removing the log file constraints
in pg_upgrade with the change you are doing here so as things would be
more consistent with non-Windows platforms, simplifying some code on the
way?  This could also be used with "vcregress upgradecheck" to see if
the concurrency is able to work its way, validating the proposed
patch...
--
Michael


signature.asc
Description: PGP signature

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

Kuntal Ghosh wrote:
> > If I use the three-argument version throughout, which should be no problem,
> > PostgreSQL builds just fine with MSVC.
> > 
> 
> I don't have enough experience on MSVC/Windows to comment on the same.
> 
> > How about checking what the buildfarm thinks about the attached?
> 
> +1

I have added it to the July commitfest.

Yours,
Laurenz Albe

Re: Loaded footgun open_datasync on Windows

[Kuntal Ghosh]

On Wed, Jun 20, 2018 at 7:36 PM, Laurenz Albe  wrote:
> Kuntal Ghosh wrote:
> [pg_test_fsync doesn't use pgwin32_open and hence doesn't respect O_DSYNC]
>> On Wed, Jun 6, 2018 at 2:39 PM, Amit Kapila  wrote:
>> > On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier  
>> > wrote:
>> > > On Wed, Jun 06, 2018 at 09:58:34AM +0530, Amit Kapila wrote:
>> > > > One another alternative could be that we
>> > > > define open as pgwin32_open (for WIN32) wherever we need it.
>> > >
>> > > Which is what basically happens on any *nix platform, are you foreseeing
>> > > anything bad here?
>> >
>> > Nothing apparent, but I think we should try to find out why at the first
>> > place this has been made backend specific.
>>
>> It seems the "#ifndef FRONTEND" restriction was added around
>> pgwin32_open() for building libpq with Visual C++ or Borland C++.  The
>> restriction was added in commit 422d4819 to build libpq with VC++[1].
>> Later, in commit fd7c3f67e0bc4, the support for Borland C++ was also
>> added.
>>
>> So, I'm not sure whether removing that restriction will work for all
>> front-end modules.
>
> Thanks for the research, and sorry for my long silence.
>
> If I remove the "#ifndef FRONTEND", building with MSVC fails for all
> call sites that use the two-argument version of open(2).
>
> If I use the three-argument version throughout, which should be no problem,
> PostgreSQL builds just fine with MSVC.
>
I don't have enough experience on MSVC/Windows to comment on the same.

> How about checking what the buildfarm thinks about the attached?
+1


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

Kuntal Ghosh wrote:
[pg_test_fsync doesn't use pgwin32_open and hence doesn't respect O_DSYNC]
> On Wed, Jun 6, 2018 at 2:39 PM, Amit Kapila  wrote:
> > On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier  
> > wrote:
> > > On Wed, Jun 06, 2018 at 09:58:34AM +0530, Amit Kapila wrote:
> > > > One another alternative could be that we
> > > > define open as pgwin32_open (for WIN32) wherever we need it.
> > > 
> > > Which is what basically happens on any *nix platform, are you foreseeing
> > > anything bad here?
> > 
> > Nothing apparent, but I think we should try to find out why at the first
> > place this has been made backend specific.
> 
> It seems the "#ifndef FRONTEND" restriction was added around
> pgwin32_open() for building libpq with Visual C++ or Borland C++.  The
> restriction was added in commit 422d4819 to build libpq with VC++[1].
> Later, in commit fd7c3f67e0bc4, the support for Borland C++ was also
> added.
> 
> So, I'm not sure whether removing that restriction will work for all
> front-end modules.

Thanks for the research, and sorry for my long silence.

If I remove the "#ifndef FRONTEND", building with MSVC fails for all
call sites that use the two-argument version of open(2).

If I use the three-argument version throughout, which should be no problem,
PostgreSQL builds just fine with MSVC.

How about checking what the buildfarm thinks about the attached?

Yours,
Laurenz AlbeFrom 5e10792cf720cedd6ebaa67cfc6163b9c5d5f305 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Wed, 20 Jun 2018 15:54:20 +0200
Subject: [PATCH] Use pgwin32_open in frontend code on Windows

This is particularly important for pg_test_fsync which does not handle
O_DSYNC correctly otherwise, leading to false claims that disks are
unsafe.

All call sites that use the two-argument form of open(2) were changed
to the three-argument form to make the Windows build succeed.
---
 src/bin/pg_basebackup/pg_receivewal.c | 2 +-
 src/bin/pg_verify_checksums/pg_verify_checksums.c | 2 +-
 src/common/file_utils.c   | 2 +-
 src/include/port.h| 3 ---
 4 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 071b32d19d..b97fce3c2b 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -287,7 +287,7 @@ FindStreamingStart(uint32 *tli)
 
 			snprintf(fullpath, sizeof(fullpath), "%s/%s", basedir, dirent->d_name);
 
-			fd = open(fullpath, O_RDONLY | PG_BINARY);
+			fd = open(fullpath, O_RDONLY | PG_BINARY, 0);
 			if (fd < 0)
 			{
 fprintf(stderr, _("%s: could not open compressed file \"%s\": %s\n"),
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 845d5aba27..efb32ffcb9 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -82,7 +82,7 @@ scan_file(char *fn, int segmentno)
 	int			f;
 	int			blockno;
 
-	f = open(fn, 0);
+	f = open(fn, 0, 0);
 	if (f < 0)
 	{
 		fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index 48876061c3..8a4cdb47ff 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -283,7 +283,7 @@ fsync_fname(const char *fname, bool isdir, const char *progname)
 	 * unsupported operations, e.g. opening a directory under Windows), and
 	 * logging others.
 	 */
-	fd = open(fname, flags);
+	fd = open(fname, flags, 0);
 	if (fd < 0)
 	{
 		if (errno == EACCES || (isdir && errno == EISDIR))
diff --git a/src/include/port.h b/src/include/port.h
index 74a9dc4d4d..2d40045ad5 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -249,11 +249,8 @@ extern bool rmtree(const char *path, bool rmtopdir);
 #define		O_DIRECT	0x8000
 extern int	pgwin32_open(const char *, int,...);
 extern FILE *pgwin32_fopen(const char *, const char *);
-
-#ifndef FRONTEND
 #define		open(a,b,c) pgwin32_open(a,b,c)
 #define		fopen(a,b) pgwin32_fopen(a,b)
-#endif
 
 /*
  * Mingw-w64 headers #define popen and pclose to _popen and _pclose.  We want
-- 
2.14.4

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Sun, Jun 10, 2018 at 10:09:26AM +0530, Amit Kapila wrote:
> As per discussion till now, we have two options to proceed:
> (a) Remove the define "#ifndef FRONTEND" that prevents pgwin32_open
> usage in frontend modules.  We have done some research and found that
> it was added in the past to allow build of modules like libpq/psql.
> If we want to use this option, then some work is needed to ensure all
> frontend modules work/behave correctly.
> (b) Use c.h in pg_test_fsync which will allow usage of pgwin32_open.
> Option (a) appears to be a good approach, but I am not sure what exact
> tests would be required.

pg_upgrade could be a good match here by removing the stuff around
SERVER_START_LOG_FILE and SERVER_STOP_LOG_FILE in pg_upgrade.h and that
can run with a single command using vcregress.bat.

> Do you prefer any of the above or have any better suggestions?

(a) looks like a good plan to me, as long as there is no back-patch as
the result patch would be likely invasive.  I would also suggest for v12
to open for commits.
--
Michael


signature.asc
Description: PGP signature

Re: Loaded footgun open_datasync on Windows

[Amit Kapila]

On Fri, Jun 8, 2018 at 11:00 PM, Magnus Hagander  wrote:
>
> On Fri, Jun 8, 2018 at 4:55 AM, Amit Kapila  wrote:
>>
>>
>> -#include "postgres_fe.h"
>> +#include "postgres.h"
>>
>> I don't think that server application can use backend environment unless
>> it is adapted to do so.  I think the application should have the capability
>> to deal with backend stuff like ereport, elog etc, if we don't want to use
>> FRONTEND environment.  The other server applications like pg_ctl.c,
>> initdb.c, etc. also uses FRONTEND environment.
>>
>
> Not having researched this particular case but yes, there are a number of
> things expected in a non-FRONTEND environment. Things that may also go
> unnoticed until too late (e.g. singal handling etc that needs explicit
> initialization). Standalong binaries should pretty much always be frontend.
>

Right, but here we are facing a situation where using FRONTEND in a
standalone binary (pg_test_fsync) won't accomplish the required
purpose.  Basically, we want to use pgwin32_open (pg specific
implementation for open on windows) in pg_test_fsync and that is
protected by "#ifndef FRONTEND".  As per discussion till now, we have
two options to proceed:
(a) Remove the define "#ifndef FRONTEND" that prevents pgwin32_open
usage in frontend modules.  We have done some research and found that
it was added in the past to allow build of modules like libpq/psql.
If we want to use this option, then some work is needed to ensure all
frontend modules work/behave correctly.
(b) Use c.h in pg_test_fsync which will allow usage of pgwin32_open.

Option (a) appears to be a good approach, but I am not sure what exact
tests would be required.  Do you prefer any of the above or have any
better suggestions?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Loaded footgun open_datasync on Windows

[Magnus Hagander]

On Fri, Jun 8, 2018 at 4:55 AM, Amit Kapila  wrote:

> On Fri, Jun 8, 2018 at 7:48 AM, Laurenz Albe 
> wrote:
>
>> Amit Kapila wrote:
>> > On Wed, Jun 6, 2018 at 3:06 PM, Kuntal Ghosh <
>> kuntalghosh.2...@gmail.com> wrote:
>> > > It seems the "#ifndef FRONTEND" restriction was added around
>> > > pgwin32_open() for building libpq with Visual C++ or Borland C++.  The
>> > > restriction was added in commit 422d4819 to build libpq with VC++[1].
>> > > Later, in commit fd7c3f67e0bc4, the support for Borland C++ was also
>> > > added.
>> > >
>> > > So, I'm not sure whether removing that restriction will work for all
>> > > front-end modules.
>> > >
>> >
>> > Thanks for doing investigation.  I agree with you that it appears from
>> the old
>> > discussion that we have added this restriction to build libpq/psql
>> (basically frontend)
>> > modules.  However, I tried to build those modules on windows by
>> removing that
>> > restriction and it doesn't give me any problem (except one extra
>> argument error,
>> > which can be easily resolved).  It might be that it gives problem with
>> certain
>> > version of MSVC.  I think if we want to pursue this direction, one
>> needs to verify
>> > on various supportted versions (of Windows) to see if things are okay.
>>
>> That's what the buildfarm is for, right?
>>
>> > Speaking about the issue at-hand, one way to make pg_test_fsync work in
>> to just
>> > include c.h and remove inclusion of postgres_fe.h, but not sure if that
>> is the best way.
>> > I am not sure if we have any other options to proceed in this case.
>> >
>> > Thoughts?
>>
>> I thought of a better way.
>> pg_test_fsync is properly listed as a server application in the
>> documentation,
>> so I think it should be built as such, as per the attached patch.
>>
>
> -#include "postgres_fe.h"
> +#include "postgres.h"
>
> I don't think that server application can use backend environment unless
> it is adapted to do so.  I think the application should have the capability
> to deal with backend stuff like ereport, elog etc, if we don't want to use
> FRONTEND environment.  The other server applications like pg_ctl.c,
> initdb.c, etc. also uses FRONTEND environment.
>
>
Not having researched this particular case but yes, there are a number of
things expected in a non-FRONTEND environment. Things that may also go
unnoticed until too late (e.g. singal handling etc that needs explicit
initialization). Standalong binaries should pretty much always be
frontend.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: Loaded footgun open_datasync on Windows

[Amit Kapila]

On Fri, Jun 8, 2018 at 7:48 AM, Laurenz Albe 
wrote:

> Amit Kapila wrote:
> > On Wed, Jun 6, 2018 at 3:06 PM, Kuntal Ghosh 
> wrote:
> > > It seems the "#ifndef FRONTEND" restriction was added around
> > > pgwin32_open() for building libpq with Visual C++ or Borland C++.  The
> > > restriction was added in commit 422d4819 to build libpq with VC++[1].
> > > Later, in commit fd7c3f67e0bc4, the support for Borland C++ was also
> > > added.
> > >
> > > So, I'm not sure whether removing that restriction will work for all
> > > front-end modules.
> > >
> >
> > Thanks for doing investigation.  I agree with you that it appears from
> the old
> > discussion that we have added this restriction to build libpq/psql
> (basically frontend)
> > modules.  However, I tried to build those modules on windows by removing
> that
> > restriction and it doesn't give me any problem (except one extra
> argument error,
> > which can be easily resolved).  It might be that it gives problem with
> certain
> > version of MSVC.  I think if we want to pursue this direction, one needs
> to verify
> > on various supportted versions (of Windows) to see if things are okay.
>
> That's what the buildfarm is for, right?
>
> > Speaking about the issue at-hand, one way to make pg_test_fsync work in
> to just
> > include c.h and remove inclusion of postgres_fe.h, but not sure if that
> is the best way.
> > I am not sure if we have any other options to proceed in this case.
> >
> > Thoughts?
>
> I thought of a better way.
> pg_test_fsync is properly listed as a server application in the
> documentation,
> so I think it should be built as such, as per the attached patch.
>

-#include "postgres_fe.h"
+#include "postgres.h"

I don't think that server application can use backend environment unless it
is adapted to do so.  I think the application should have the capability to
deal with backend stuff like ereport, elog etc, if we don't want to use
FRONTEND environment.  The other server applications like pg_ctl.c,
initdb.c, etc. also uses FRONTEND environment.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

Amit Kapila wrote:
> On Wed, Jun 6, 2018 at 3:06 PM, Kuntal Ghosh  
> wrote:
> > It seems the "#ifndef FRONTEND" restriction was added around
> > pgwin32_open() for building libpq with Visual C++ or Borland C++.  The
> > restriction was added in commit 422d4819 to build libpq with VC++[1].
> > Later, in commit fd7c3f67e0bc4, the support for Borland C++ was also
> > added.
> > 
> > So, I'm not sure whether removing that restriction will work for all
> > front-end modules.
> > 
> 
> Thanks for doing investigation.  I agree with you that it appears from the old
> discussion that we have added this restriction to build libpq/psql (basically 
> frontend)
> modules.  However, I tried to build those modules on windows by removing that
> restriction and it doesn't give me any problem (except one extra argument 
> error,
> which can be easily resolved).  It might be that it gives problem with certain
> version of MSVC.  I think if we want to pursue this direction, one needs to 
> verify
> on various supportted versions (of Windows) to see if things are okay.

That's what the buildfarm is for, right?

> Speaking about the issue at-hand, one way to make pg_test_fsync work in to 
> just
> include c.h and remove inclusion of postgres_fe.h, but not sure if that is 
> the best way.
> I am not sure if we have any other options to proceed in this case.
> 
> Thoughts?

I thought of a better way.
pg_test_fsync is properly listed as a server application in the documentation,
so I think it should be built as such, as per the attached patch.

Yours,
Laurenz AlbeFrom 032a0463072097e489f912337ea08f7373a4f107 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Fri, 8 Jun 2018 04:07:55 +0200
Subject: [PATCH] Build pg_test_fsync as backend code

Including "postgres.h" rather than "postgres_fe.h" makes
pg_test_fsync use pgwin32_open() on Windows, which is the
proper thing to do because it should test the behavior on
the backend side.
---
 src/bin/pg_test_fsync/pg_test_fsync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index e6f7ef8557..c1e1d7ef38 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -3,7 +3,7 @@
  *		tests all supported fsync() methods
  */
 
-#include "postgres_fe.h"
+#include "postgres.h"
 
 #include 
 #include 
-- 
2.14.4

Re: Loaded footgun open_datasync on Windows

[Amit Kapila]

On Wed, Jun 6, 2018 at 3:06 PM, Kuntal Ghosh 
wrote:

> On Wed, Jun 6, 2018 at 2:39 PM, Amit Kapila 
> wrote:
> > On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier 
> > wrote:
> >>
> >> On Wed, Jun 06, 2018 at 09:58:34AM +0530, Amit Kapila wrote:
> >>
> >> >> It could be
> >> >> risky for existing callers of open() for tool maintainers, or on the
> >> >> contrary people could welcome a wrapper of open() which is
> >> >> concurrent-safe in their own tools.
> >> >
> >> > I am not sure if we can safely assume that because using these
> functions
> >> > would allow users to concurrently delete the files, but may be it is
> >> > okay
> >> > for all the FRONTEND modules.  One another alternative could be that
> we
> >> > define open as pgwin32_open (for WIN32) wherever we need it.
> >>
> >> Which is what basically happens on any *nix platform, are you foreseeing
> >> anything bad here?
> >>
> >>
> >
> > Nothing apparent, but I think we should try to find out why at the first
> > place this has been made backend specific.
> >
> It seems the "#ifndef FRONTEND" restriction was added around
> pgwin32_open() for building libpq with Visual C++ or Borland C++.  The
> restriction was added in commit 422d4819 to build libpq with VC++[1].
> Later, in commit fd7c3f67e0bc4, the support for Borland C++ was also
> added.
>
> So, I'm not sure whether removing that restriction will work for all
> front-end modules.
>
>
Thanks for doing investigation.  I agree with you that it appears from the
old discussion that we have added this restriction to build libpq/psql
(basically frontend) modules.  However, I tried to build those modules on
windows by removing that restriction and it doesn't give me any problem
(except one extra argument error, which can be easily resolved).  It might
be that it gives problem with certain version of MSVC.  I think if we want
to pursue this direction, one needs to verify on various supportted
versions (of Windows) to see if things are okay.

Speaking about the issue at-hand, one way to make pg_test_fsync work in to
just include c.h and remove inclusion of postgres_fe.h, but not sure if
that is the best way.  I am not sure if we have any other options to
proceed in this case.

Thoughts?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Loaded footgun open_datasync on Windows

[Kuntal Ghosh]

On Wed, Jun 6, 2018 at 2:39 PM, Amit Kapila  wrote:
> On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier 
> wrote:
>>
>> On Wed, Jun 06, 2018 at 09:58:34AM +0530, Amit Kapila wrote:
>>
>> >> It could be
>> >> risky for existing callers of open() for tool maintainers, or on the
>> >> contrary people could welcome a wrapper of open() which is
>> >> concurrent-safe in their own tools.
>> >
>> > I am not sure if we can safely assume that because using these functions
>> > would allow users to concurrently delete the files, but may be it is
>> > okay
>> > for all the FRONTEND modules.  One another alternative could be that we
>> > define open as pgwin32_open (for WIN32) wherever we need it.
>>
>> Which is what basically happens on any *nix platform, are you foreseeing
>> anything bad here?
>>
>>
>
> Nothing apparent, but I think we should try to find out why at the first
> place this has been made backend specific.
>
It seems the "#ifndef FRONTEND" restriction was added around
pgwin32_open() for building libpq with Visual C++ or Borland C++.  The
restriction was added in commit 422d4819 to build libpq with VC++[1].
Later, in commit fd7c3f67e0bc4, the support for Borland C++ was also
added.

So, I'm not sure whether removing that restriction will work for all
front-end modules.

[1] 
https://www.postgresql.org/message-id/flat/D90A5A6C612A39408103E6ECDD77B829408D4F%40voyager.corporate.connx.com#d90a5a6c612a39408103e6ecdd77b829408...@voyager.corporate.connx.com
-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

Re: Loaded footgun open_datasync on Windows

[Amit Kapila]

On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier 
wrote:

> On Wed, Jun 06, 2018 at 09:58:34AM +0530, Amit Kapila wrote:
>
> >> It could be
> >> risky for existing callers of open() for tool maintainers, or on the
> >> contrary people could welcome a wrapper of open() which is
> >> concurrent-safe in their own tools.
> >
> > I am not sure if we can safely assume that because using these functions
> > would allow users to concurrently delete the files, but may be it is okay
> > for all the FRONTEND modules.  One another alternative could be that we
> > define open as pgwin32_open (for WIN32) wherever we need it.
>
> Which is what basically happens on any *nix platform, are you foreseeing
> anything bad here?


>
Nothing apparent, but I think we should try to find out why at the first
place this has been made backend specific.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Wed, Jun 06, 2018 at 09:58:34AM +0530, Amit Kapila wrote:
> On Wed, Jun 6, 2018 at 8:28 AM, Michael Paquier  wrote:
>> Ouch.  Including directly c.h as you do here is against project policy
>> code.  See recent commit a72f0365 for example.  pgwin32_open() is
>> visibly able to handle frontend code if I read this code correctly, so
>> could we just remove the "#ifndef FRONTEND" restriction?
> 
> I think we need to explore a bit, if we want to remove that, for example
> some of the frontend modules (like pg_receivewal, pg_recvlogical) call two
> argument open which would require change.

Yeah, sure.  I am not saying that this is straight-forward, but it may
be worth the switch in the long term.  What I am sure about is that the
proposed patch is simple, but that does not look like a correct
approach to me as other tools may benefit from it.

>> It could be
>> risky for existing callers of open() for tool maintainers, or on the
>> contrary people could welcome a wrapper of open() which is
>> concurrent-safe in their own tools.
> 
> I am not sure if we can safely assume that because using these functions
> would allow users to concurrently delete the files, but may be it is okay
> for all the FRONTEND modules.  One another alternative could be that we
> define open as pgwin32_open (for WIN32) wherever we need it.

Which is what basically happens on any *nix platform, are you foreseeing
anything bad here?  Windows has the characteristic in being particular
in everything, which is its main characteristic, so I may be missing
something of course.
--
Michael


signature.asc
Description: PGP signature

Re: Loaded footgun open_datasync on Windows

[Amit Kapila]

On Wed, Jun 6, 2018 at 8:28 AM, Michael Paquier  wrote:

> On Tue, Jun 05, 2018 at 04:15:00PM +0200, Laurenz Albe wrote:
>
> > --- a/src/bin/pg_test_fsync/pg_test_fsync.c
> > +++ b/src/bin/pg_test_fsync/pg_test_fsync.c
> > @@ -3,6 +3,8 @@
> >   *   tests all supported fsync() methods
> >   */
> >
> > +/* we have to include c.h first so that pgwin32_open is used on Windows
> */
> > +#include "c.h"
> >  #include "postgres_fe.h"
> >
> >  #include 
>
> Ouch.  Including directly c.h as you do here is against project policy
> code.  See recent commit a72f0365 for example.  pgwin32_open() is
> visibly able to handle frontend code if I read this code correctly, so
> could we just remove the "#ifndef FRONTEND" restriction?


I think we need to explore a bit, if we want to remove that, for example
some of the frontend modules (like pg_receivewal, pg_recvlogical) call two
argument open which would require change.



> It could be
> risky for existing callers of open() for tool maintainers, or on the
> contrary people could welcome a wrapper of open() which is
> concurrent-safe in their own tools.


I am not sure if we can safely assume that because using these functions
would allow users to concurrently delete the files, but may be it is okay
for all the FRONTEND modules.  One another alternative could be that we
define open as pgwin32_open (for WIN32) wherever we need it.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Tue, Jun 05, 2018 at 04:15:00PM +0200, Laurenz Albe wrote:
> The attached patch makes pg_test_fsync use pgwin32_open on Windows, which is 
> what
> we use elsewhere.

Well, pg_upgrade may benefit from that as one example, as any other
tools.

> That should fix the problem.
> Ist there a better way to do this?  The problem is that "c.h" is only included
> at the very end of "postgres-fe.h", which makes front-end code use "open"
> rather than "pgwin32_open" on Windows.

And port.h is included at the end of c.h.

> Having read it again, I think that the documentation is fine as it is:
> After all, this is just advice what you can do if you are running on unsafe 
> hardware,
> which doesn't flush to disk like it should.

People tend to ignore this part from the docs.  Well I won't fight hard
on that if folks don't want to change that...

> --- a/src/bin/pg_test_fsync/pg_test_fsync.c
> +++ b/src/bin/pg_test_fsync/pg_test_fsync.c
> @@ -3,6 +3,8 @@
>   *   tests all supported fsync() methods
>   */
>  
> +/* we have to include c.h first so that pgwin32_open is used on Windows */
> +#include "c.h"
>  #include "postgres_fe.h"
>  
>  #include 

Ouch.  Including directly c.h as you do here is against project policy
code.  See recent commit a72f0365 for example.  pgwin32_open() is
visibly able to handle frontend code if I read this code correctly, so
could we just remove the "#ifndef FRONTEND" restriction?  It could be
risky for existing callers of open() for tool maintainers, or on the
contrary people could welcome a wrapper of open() which is
concurrent-safe in their own tools.  I am not sure which position is
better here, but thinking that all in-core frontend code could benefit
from a couple of simplifications that could be worth the shot.
--
Michael


signature.asc
Description: PGP signature

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

Amit Kapila wrote:
> On Fri, Jun 1, 2018 at 8:18 PM, Laurenz Albe  wrote:
> > Amit Kapila wrote:
> > > On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe  
> > > wrote:
> > > > I recently read our documentation about reliability on Windows:
> > > > 
> > > > > On Windows, if wal_sync_method is open_datasync (the default), write 
> > > > > caching can
> > > > > be disabled by unchecking
> > > > > My Computer\Open\disk 
> > > > > drive\Properties\Hardware\Properties\Policies\Enable write caching
> > > > > on the disk. Alternatively, set wal_sync_method to fsync or 
> > > > > fsync_writethrough,
> > > > > which prevent write caching.
> > > > 
> > > > It seems dangerous to me to initialize "wal_sync_method" to a method 
> > > > that is unsafe
> > > > by default.  Admittedly I am not a Windows man, but the fact that this 
> > > > has eluded me
> > > > up to now leads me to believe that other people running PostgreSQL on 
> > > > Windows might
> > > > also have missed that important piece of advice and are consequently 
> > > > running with
> > > > an unsafe setup.
> > > > 
> > > > Wouldn't it be smarter to set a different default value on Windows, 
> > > > like we do on
> > > > Linux (for other reasons)?
> > > > 
> > > 
> > > One thing to note is that it seems that in code we use 
> > > FILE_FLAG_WRITE_THROUGH for
> > > open_datasync which according to MSDN [1] will bypass any intermediate 
> > > cache .
> > > See pgwin32_open.  Have you experimented to set any other option as we 
> > > have a comment
> > > in code which say Win32 only has O_DSYNC?
> > > 
> > > 
> > > [1] - 
> > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx
> > 
> > After studying the code I feel somewhat safer; it looks like the code is ok.
> > I have no Windows at hand, so I cannot test right now.
> > 
> > What happened is that I ran "pg_test_fsync" at a customer site on Windows, 
> > and
> > it returned ridiculously high rates got open_datasync.
> > 
> > So I think that the following should be fixed:
> > 
> > - Change pg_test_fsync to actually use FILE_FLAG_WRITE_THROUGH.
> 
> It sounds sensible to me to make a Windows specific change in pg_test_fsync 
> for open_datasync method.
> That will make pg_test_fsync behave similar to server.

The attached patch makes pg_test_fsync use pgwin32_open on Windows, which is 
what
we use elsewhere.  That should fix the problem.
Ist there a better way to do this?  The problem is that "c.h" is only included
at the very end of "postgres-fe.h", which makes front-end code use "open"
rather than "pgwin32_open" on Windows.

Having read it again, I think that the documentation is fine as it is:
After all, this is just advice what you can do if you are running on unsafe 
hardware,
which doesn't flush to disk like it should.

Yours,
Laurenz AlbeFrom 6ab651c48df66ec93b8d6730bebeaf60e2bb865b Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Tue, 5 Jun 2018 16:05:10 +0200
Subject: [PATCH] Make pg_test_fsync use pgwin32_open on Windows

---
 src/bin/pg_test_fsync/pg_test_fsync.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index e6f7ef8557..a0139a1302 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -3,6 +3,8 @@
  *		tests all supported fsync() methods
  */
 
+/* we have to include c.h first so that pgwin32_open is used on Windows */
+#include "c.h"
 #include "postgres_fe.h"
 
 #include 
-- 
2.14.3

Re: Loaded footgun open_datasync on Windows

[Robert Haas]

On Fri, Jun 1, 2018 at 2:56 PM, Michael Paquier  wrote:
> When things come to VMs or containers, developers and users tend to be
> sloppy regarding the hardware being used, and they are not usually aware
> that the database running within it is sensitive to such details.  Many
> folks use Postgres, so IMO having defaults which are safe without
> relying on hardware characteristics is a rather sensible approach. (Got
> bitten by that in the past, not everybody is careful).  My 2c.

Maybe we should also adopt that approach on macOS.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Fri, Jun 01, 2018 at 07:32:26PM +0200, Magnus Hagander wrote:
> Basically, this method *is* safe as long as you have proper storage. For
> example, if yo have a RAID controller with cache, it is perfectly safe. If
> you have a consumer level device with unsafe caching, then it's not safe.
> This behaves basically the same as it does on e.g. Linux, which is also
> unsafe if you have an unsafe conusmer device.

When things come to VMs or containers, developers and users tend to be
sloppy regarding the hardware being used, and they are not usually aware
that the database running within it is sensitive to such details.  Many
folks use Postgres, so IMO having defaults which are safe without
relying on hardware characteristics is a rather sensible approach. (Got
bitten by that in the past, not everybody is careful).  My 2c.
--
Michael


signature.asc
Description: PGP signature

Re: Loaded footgun open_datasync on Windows

[Amit Kapila]

On Fri, Jun 1, 2018 at 8:18 PM, Laurenz Albe 
wrote:

> Amit Kapila wrote:
> > On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe 
> wrote:
> > > I recently read our documentation about reliability on Windows:
> > >
> > > > On Windows, if wal_sync_method is open_datasync (the default), write
> caching can
> > > > be disabled by unchecking
> > > > My Computer\Open\disk 
> > > > drive\Properties\Hardware\Properties\Policies\Enable
> write caching
> > > > on the disk. Alternatively, set wal_sync_method to fsync or
> fsync_writethrough,
> > > > which prevent write caching.
> > >
> > > It seems dangerous to me to initialize "wal_sync_method" to a method
> that is unsafe
> > > by default.  Admittedly I am not a Windows man, but the fact that this
> has eluded me
> > > up to now leads me to believe that other people running PostgreSQL on
> Windows might
> > > also have missed that important piece of advice and are consequently
> running with
> > > an unsafe setup.
> > >
> > > Wouldn't it be smarter to set a different default value on Windows,
> like we do on
> > > Linux (for other reasons)?
> > >
> >
> > One thing to note is that it seems that in code we use
> FILE_FLAG_WRITE_THROUGH for
> > open_datasync which according to MSDN [1] will bypass any intermediate
> cache .
> > See pgwin32_open.  Have you experimented to set any other option as we
> have a comment
> > in code which say Win32 only has O_DSYNC?
> >
> >
> > [1] - https://msdn.microsoft.com/en-us/library/windows/desktop/
> aa363858(v=vs.85).aspx
>
> After studying the code I feel somewhat safer; it looks like the code is
> ok.
> I have no Windows at hand, so I cannot test right now.
>
> What happened is that I ran "pg_test_fsync" at a customer site on Windows,
> and
> it returned ridiculously high rates got open_datasync.
>
> So I think that the following should be fixed:
>
> - Change pg_test_fsync to actually use FILE_FLAG_WRITE_THROUGH.
>

It sounds sensible to me to make a Windows specific change in pg_test_fsync
for open_datasync method.  That will make pg_test_fsync behave similar to
server.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Loaded footgun open_datasync on Windows

[Magnus Hagander]

On Fri, Jun 1, 2018 at 3:26 PM, Amit Kapila  wrote:

> On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe 
> wrote:
>
>> I recently read our documentation about reliability on Windows:
>>
>> > On Windows, if wal_sync_method is open_datasync (the default), write
>> caching can
>> > be disabled by unchecking
>> > My Computer\Open\disk drive\Properties\Hardware\Properties\Policies\Enable
>> write caching
>> > on the disk. Alternatively, set wal_sync_method to fsync or
>> fsync_writethrough,
>> > which prevent write caching.
>>
>> It seems dangerous to me to initialize "wal_sync_method" to a method that
>> is unsafe
>> by default.  Admittedly I am not a Windows man, but the fact that this
>> has eluded me
>> up to now leads me to believe that other people running PostgreSQL on
>> Windows might
>> also have missed that important piece of advice and are consequently
>> running with
>> an unsafe setup.
>>
>> Wouldn't it be smarter to set a different default value on Windows, like
>> we do on
>> Linux (for other reasons)?
>>
>>
> One thing to note is that it seems that in code we use
> FILE_FLAG_WRITE_THROUGH for open_datasync which according to MSDN [1] will
> bypass any intermediate cache .  See pgwin32_open.  Have you experimented
> to set any other option as we have a comment in code which say Win32 only
> has O_DSYNC?
>
>
These settings go back to the original Windows port, and it would probably
be a good idea to read back on the discusison there (sorry, I don't have a
direct reference to it here).

Basically, this method *is* safe as long as you have proper storage. For
example, if yo have a RAID controller with cache, it is perfectly safe. If
you have a consumer level device with unsafe caching, then it's not safe.
This behaves basically the same as it does on e.g. Linux, which is also
unsafe if you have an unsafe conusmer device.

If you use fsync_writethrough, we actually write through the cache on the
raidcontroller *even if it has bettery/flash cache*, which gives absolutely
terrible performance on these platforms. It is useful if you have a
consumer drive that by default does insafe caching but does respect
writethrough requests.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

Amit Kapila wrote:
> On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe  wrote:
> > I recently read our documentation about reliability on Windows:
> > 
> > > On Windows, if wal_sync_method is open_datasync (the default), write 
> > > caching can
> > > be disabled by unchecking
> > > My Computer\Open\disk 
> > > drive\Properties\Hardware\Properties\Policies\Enable write caching
> > > on the disk. Alternatively, set wal_sync_method to fsync or 
> > > fsync_writethrough,
> > > which prevent write caching.
> > 
> > It seems dangerous to me to initialize "wal_sync_method" to a method that 
> > is unsafe
> > by default.  Admittedly I am not a Windows man, but the fact that this has 
> > eluded me
> > up to now leads me to believe that other people running PostgreSQL on 
> > Windows might
> > also have missed that important piece of advice and are consequently 
> > running with
> > an unsafe setup.
> > 
> > Wouldn't it be smarter to set a different default value on Windows, like we 
> > do on
> > Linux (for other reasons)?
> > 
> 
> One thing to note is that it seems that in code we use 
> FILE_FLAG_WRITE_THROUGH for
> open_datasync which according to MSDN [1] will bypass any intermediate cache .
> See pgwin32_open.  Have you experimented to set any other option as we have a 
> comment
> in code which say Win32 only has O_DSYNC?
> 
> 
> [1] - 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx

After studying the code I feel somewhat safer; it looks like the code is ok.
I have no Windows at hand, so I cannot test right now.

What happened is that I ran "pg_test_fsync" at a customer site on Windows, and
it returned ridiculously high rates got open_datasync.

So I think that the following should be fixed:

- Change pg_test_fsync to actually use FILE_FLAG_WRITE_THROUGH.
  Currently it uses O_DSYNC, which is defined in port/win32_port.h as

  /*
   * Supplement to .
   * This is the same value as _O_NOINHERIT in the MS header file. This is
   * to ensure that we don't collide with a future definition. It means
   * we cannot use _O_NOINHERIT ourselves.
   */
  #define O_DSYNC 0x0080

- Change the documentation so that it does not claim that open_datasync is 
unsafe
  unless you change the device settings.

If there is a consensus that this is fine, I can do the legwork.

Yours,
Laurenz Albe

Re: Loaded footgun open_datasync on Windows

[Amit Kapila]

On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe 
wrote:

> I recently read our documentation about reliability on Windows:
>
> > On Windows, if wal_sync_method is open_datasync (the default), write
> caching can
> > be disabled by unchecking
> > My Computer\Open\disk drive\Properties\Hardware\Properties\Policies\Enable
> write caching
> > on the disk. Alternatively, set wal_sync_method to fsync or
> fsync_writethrough,
> > which prevent write caching.
>
> It seems dangerous to me to initialize "wal_sync_method" to a method that
> is unsafe
> by default.  Admittedly I am not a Windows man, but the fact that this has
> eluded me
> up to now leads me to believe that other people running PostgreSQL on
> Windows might
> also have missed that important piece of advice and are consequently
> running with
> an unsafe setup.
>
> Wouldn't it be smarter to set a different default value on Windows, like
> we do on
> Linux (for other reasons)?
>
>
One thing to note is that it seems that in code we use
FILE_FLAG_WRITE_THROUGH for open_datasync which according to MSDN [1] will
bypass any intermediate cache .  See pgwin32_open.  Have you experimented
to set any other option as we have a comment in code which say Win32 only
has O_DSYNC?


[1] -
https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Fri, Jun 01, 2018 at 11:43:33AM +0200, Laurenz Albe wrote:
> I am worried that there might be loads of Windows installations out there 
> happily
> running productive databases with an unsafe setup, so I'd even suggest 
> backpatching
> such a change.

This is not only your imagination, there are deployments doing so, for
the sole reasons that the default values are "safe" :)

I would be all for it for changing the default value on HEAD to a safe
value as well as the back-branches.
--
Michael


signature.asc
Description: PGP signature

Loaded footgun open_datasync on Windows

[Laurenz Albe]

I recently read our documentation about reliability on Windows:

> On Windows, if wal_sync_method is open_datasync (the default), write caching 
> can
> be disabled by unchecking
> My Computer\Open\disk drive\Properties\Hardware\Properties\Policies\Enable 
> write caching
> on the disk. Alternatively, set wal_sync_method to fsync or 
> fsync_writethrough,
> which prevent write caching.

It seems dangerous to me to initialize "wal_sync_method" to a method that is 
unsafe
by default.  Admittedly I am not a Windows man, but the fact that this has 
eluded me
up to now leads me to believe that other people running PostgreSQL on Windows 
might
also have missed that important piece of advice and are consequently running 
with
an unsafe setup.

Wouldn't it be smarter to set a different default value on Windows, like we do 
on
Linux (for other reasons)?

I am worried that there might be loads of Windows installations out there 
happily
running productive databases with an unsafe setup, so I'd even suggest 
backpatching
such a change.

Yours,
Laurenz Albe