Loaded footgun open_datasync on Windows

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
37 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Loaded footgun open_datasync on Windows

Laurenz Albe
I recently read our documentation about reliability on Windows:

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

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

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

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

Yours,
Laurenz Albe

Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

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

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

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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

akapila
In reply to this post by Laurenz Albe
On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe <[hidden email]> 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?


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

Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

Laurenz Albe
Amit Kapila wrote:

> On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

Magnus Hagander-2
In reply to this post by akapila

On Fri, Jun 1, 2018 at 3:26 PM, Amit Kapila <[hidden email]> wrote:
On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe <[hidden email]> wrote:
I recently read our documentation about reliability on Windows:

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

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

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


One thing to note is that it seems that in code we use FILE_FLAG_WRITE_THROUGH for open_datasync which according to MSDN [1] will bypass any intermediate cache .  See pgwin32_open.  Have you experimented to set any other option as we have a comment in code which say Win32 only has O_DSYNC?


These settings go back to the original Windows port, and it would probably be a good idea to read back on the discusison there (sorry, I don't have a direct reference to it here).

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

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

--
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

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

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

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

So I think that the following should be fixed:

- Change pg_test_fsync to actually use FILE_FLAG_WRITE_THROUGH.

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

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

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

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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

Robert Haas
On Fri, Jun 1, 2018 at 2:56 PM, Michael Paquier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

Laurenz Albe
In reply to this post by akapila
Amit Kapila wrote:

> On Fri, Jun 1, 2018 at 8:18 PM, Laurenz Albe <[hidden email]> wrote:
> > Amit Kapila wrote:
> > > On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe <[hidden email]> 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

0001-Make-pg_test_fsync-use-pgwin32_open-on-Windows.patch (1012 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

akapila
On Wed, Jun 6, 2018 at 8:28 AM, Michael Paquier <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

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

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

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

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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

Thomas Munro-3
On Wed, Jun 6, 2018 at 4:48 PM, Michael Paquier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

akapila
In reply to this post by Michael Paquier-2
On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

Kuntal Ghosh
On Wed, Jun 6, 2018 at 2:39 PM, Amit Kapila <[hidden email]> wrote:

> On Wed, Jun 6, 2018 at 10:18 AM, Michael Paquier <[hidden email]>
> 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@...
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

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

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


Thanks for doing investigation.  I agree with you that it appears from the old discussion that we have added this restriction to build libpq/psql (basically frontend) modules.  However, I tried to build those modules on windows by removing that restriction and it doesn't give me any problem (except one extra argument error, which can be easily resolved).  It might be that it gives problem with certain version of MSVC.  I think if we want to pursue this direction, one needs to verify on various supportted versions (of Windows) to see if things are okay.

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

Thoughts?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

Laurenz Albe
Amit Kapila wrote:

> On Wed, Jun 6, 2018 at 3:06 PM, Kuntal Ghosh <[hidden email]> 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

0001-Build-pg_test_fsync-as-backend-code.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

akapila
On Fri, Jun 8, 2018 at 7:48 AM, Laurenz Albe <[hidden email]> wrote:
Amit Kapila wrote:
> On Wed, Jun 6, 2018 at 3:06 PM, Kuntal Ghosh <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

Magnus Hagander-2


On Fri, Jun 8, 2018 at 4:55 AM, Amit Kapila <[hidden email]> wrote:
On Fri, Jun 8, 2018 at 7:48 AM, Laurenz Albe <[hidden email]> wrote:
Amit Kapila wrote:
> On Wed, Jun 6, 2018 at 3:06 PM, Kuntal Ghosh <[hidden email]> 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.  


--
Reply | Threaded
Open this post in threaded view
|

Re: Loaded footgun open_datasync on Windows

akapila
On Fri, Jun 8, 2018 at 11:00 PM, Magnus Hagander <[hidden email]> wrote:

>
> On Fri, Jun 8, 2018 at 4:55 AM, Amit Kapila <[hidden email]> 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

12