Обсуждение: 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

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 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

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 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:

https://www.postgresql.org/message-id/flat/16922.1520722108%40sss.pgh.pa.us

-- 
Thomas Munro
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].
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#D90A5A6C612A39408103E6ECDD77B829408D4F@voyager.corporate.connx.com
-- 
Thanks & Regards,
Kuntal Ghosh
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

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

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

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

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.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 https://postgr.es/m/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

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 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