Re: Loaded footgun open_datasync on Windows

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