Loaded footgun open_datasync on Windows

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\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]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\Enable

write caching

on the disk. Alternatively, set wal_sync_method to fsync or

fsync_writethrough,

which prevent write caching.

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

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

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

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

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

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

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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.

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

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

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 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/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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