Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi

On Mon, Sep 17, 2018 at 07:38:24PM -0400, Tom Lane wrote:
> So we seem to be out of the woods in terms of 0ba06e0bf breaking the
> regression tests, but I'm not very happy about the whole thing, because
> that patch wasn't supposed to change the behavior of open/fopen in any
> way other than allowing concurrent file access. Obviously, it did.

Thanks! I can see that as well.

> After looking at src/port/open.c a bit, it seems like the problem is
> that our fopen() requires a nonstandard "t" option to select text mode.
> I'd always supposed that you got binary mode with "b" and text mode
> otherwise; is there some third possibility on Windows?

There is a sort of automatic mode... Please see below.

> Anyway, I'm inclined to propose that we ought to do something like
> the attached in HEAD and v11 in order to reduce the risk that there
> are other unexpected behavioral changes. This should also let us
> revert 0ba06e0bf's change in initdb.c.
>
> I wonder whether pgwin32_open() also ought to enforce that either
> O_BINARY or O_TEXT mode gets selected.

There is no need to go down that road I think, a lot of code paths
already call setmode to make sure that binary or text modes are used.

> diff --git a/src/port/open.c b/src/port/open.c
> index a3ad946..85ab06e 100644
> --- a/src/port/open.c
> +++ b/src/port/open.c
> @@ -154,7 +154,7 @@ pgwin32_fopen(const char *fileName, const char *mode)
>
> if (strchr(mode, 'b'))
> openmode |= O_BINARY;
> - if (strchr(mode, 't'))
> + else
> openmode |= O_TEXT;

Hm. I don't think that this is correct either. The whole logic behind
the mode selected depends on how setmode() is being set to. Please see
here:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/setmode?view=vs-2017

And the point is that if neither 'b' nor 't' are set, then open() would
use the value set by the process, which is 't' by default if not set.
And initdb does not set that, so it would use 't'. miscinit.c sets it
to 'b', and your patch would likely cause some backend code to be
broken. So it seems to me that if 'b' or 't' are not defined by the
caller, then we ought to use what get_fmode() returns:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/get-fmode?view=vs-2017

What I think I broke is that CreateFile ignores what _fmode uses, which
has caused the breakage, while calling directly open() or fopen() does
the work. There are also other things assuming that binary mode is
used, you can grep for "setmode" and see how miscinit.c or pg_basebackup
do the job.
--
Michael