Loaded footgun open_datasync on Windows
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
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
On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe <laurenz.albe@cybertec.at>
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\Enablewrite 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]https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx 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
https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Amit Kapila wrote:
On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe <laurenz.albe@cybertec.at> 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 <fcntl.h>.
* 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
On Fri, Jun 1, 2018 at 3:26 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe <laurenz.albe@cybertec.at>
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\Enablewrite 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/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
On Fri, Jun 1, 2018 at 8:18 PM, Laurenz Albe <laurenz.albe@cybertec.at>
wrote:
Amit Kapila wrote:
On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe <laurenz.albe@cybertec.at>
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\Enablewrite 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
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
On Fri, Jun 1, 2018 at 2:56 PM, Michael Paquier <michael@paquier.xyz> 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
Amit Kapila wrote:
On Fri, Jun 1, 2018 at 8:18 PM, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
Amit Kapila wrote:
On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe <laurenz.albe@cybertec.at> 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 Albe
Attachments:
0001-Make-pg_test_fsync-use-pgwin32_open-on-Windows.patchtext/x-patch; charset=UTF-8; name=0001-Make-pg_test_fsync-use-pgwin32_open-on-Windows.patchDownload
From 6ab651c48df66ec93b8d6730bebeaf60e2bb865b Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
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 <sys/stat.h>
--
2.14.3
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 <sys/stat.h>
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
On Wed, Jun 6, 2018 at 8:28 AM, Michael Paquier <michael@paquier.xyz> 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 <sys/stat.h>
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
On Wed, Jun 06, 2018 at 09:58:34AM +0530, Amit Kapila wrote:
On Wed, Jun 6, 2018 at 8:28 AM, Michael Paquier <michael@paquier.xyz> 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
On Wed, Jun 6, 2018 at 4:48 PM, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 06, 2018 at 09:58:34AM +0530, Amit Kapila wrote:
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.
Like this other unresolved problem:
/messages/by-id/16922.1520722108@sss.pgh.pa.us
--
Thomas Munro
http://www.enterprisedb.com
On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier <michael@paquier.xyz>
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
On Wed, Jun 6, 2018 at 2:39 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier <michael@paquier.xyz>
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]/messages/by-id/D90A5A6C612A39408103E6ECDD77B829408D4F@voyager.corporate.connx.com -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com.
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]: /messages/by-id/D90A5A6C612A39408103E6ECDD77B829408D4F@voyager.corporate.connx.com -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
On Wed, Jun 6, 2018 at 3:06 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com>
wrote:
On Wed, Jun 6, 2018 at 2:39 PM, Amit Kapila <amit.kapila16@gmail.com>
wrote:On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier <michael@paquier.xyz>
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 thatwe
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
Amit Kapila wrote:
On Wed, Jun 6, 2018 at 3:06 PM, Kuntal Ghosh <kuntalghosh.2007@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.
Yours,
Laurenz Albe
Attachments:
0001-Build-pg_test_fsync-as-backend-code.patchtext/x-patch; charset=UTF-8; name=0001-Build-pg_test_fsync-as-backend-code.patchDownload
From 032a0463072097e489f912337ea08f7373a4f107 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
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 <sys/stat.h>
#include <sys/time.h>
--
2.14.4
On Fri, Jun 8, 2018 at 7:48 AM, Laurenz Albe <laurenz.albe@cybertec.at>
wrote:
Amit Kapila wrote:
On Wed, Jun 6, 2018 at 3:06 PM, Kuntal Ghosh <kuntalghosh.2007@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.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Fri, Jun 8, 2018 at 4:55 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Fri, Jun 8, 2018 at 7:48 AM, Laurenz Albe <laurenz.albe@cybertec.at>
wrote:Amit Kapila wrote:
On Wed, Jun 6, 2018 at 3:06 PM, Kuntal Ghosh <
kuntalghosh.2007@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/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
On Fri, Jun 8, 2018 at 11:00 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Fri, Jun 8, 2018 at 4:55 AM, Amit Kapila <amit.kapila16@gmail.com> 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
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
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 <amit.kapila16@gmail.com> wrote:
On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier <michael@paquier.xyz> 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 Albe
Attachments:
0001-Use-pgwin32_open-in-frontend-code-on-Windows.patchtext/x-patch; charset=UTF-8; name=0001-Use-pgwin32_open-in-frontend-code-on-Windows.patchDownload
From 5e10792cf720cedd6ebaa67cfc6163b9c5d5f305 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
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 0x80000000
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
On Wed, Jun 20, 2018 at 7:36 PM, Laurenz Albe <laurenz.albe@cybertec.at> 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 <amit.kapila16@gmail.com> wrote:
On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier <michael@paquier.xyz> 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
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
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
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
On Thu, Jun 21, 2018 at 2:06 AM, Laurenz Albe <laurenz.albe@cybertec.at> 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
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 Albe
Attachments:
0001-Use-pgwin32_open-in-frontend-code-on-Windows_V2.patchtext/x-patch; charset=UTF-8; name=0001-Use-pgwin32_open-in-frontend-code-on-Windows_V2.patchDownload
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 0x80000000
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
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.
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
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
Attachments:
0001-Use-pgwin32_open-in-frontend-code-on-Windows_V3.patchtext/x-patch; charset=UTF-8; name=0001-Use-pgwin32_open-in-frontend-code-on-Windows_V3.patchDownload
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 0x80000000
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
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 /messages/by-id/16922.1520722108@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
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 /messages/by-id/16922.1520722108@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
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
Attachments:
0001-Use-pgwin32_open-in-frontend-code-on-Windows_V4.patchtext/x-patch; charset=UTF-8; name=0001-Use-pgwin32_open-in-frontend-code-on-Windows_V4.patchDownload
From 8cd3d1a75fc3b18e6928b9b26841f1947d1f72ba Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
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 (fd < 0)
{
if (errno == EACCES || (isdir && errno == EISDIR))
diff --git a/src/include/port.h b/src/include/port.h
index d92756111f..2522ebc35f 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 0x80000000
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.17.0
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
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
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
On Fri, Sep 14, 2018 at 3:32 AM Michael Paquier <michael@paquier.xyz> 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
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