Date: Tue, 18 Sep 2018 09:11:43 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code
 for Wi
Message-ID: <20180918001143.GG31460@paquier.xyz>
References: <E1g0cYN-0003oE-IE@gemulon.postgresql.org>
 <23543.1536938575@sss.pgh.pa.us>
 <20180914224939.GA1178@paquier.xyz>
 <20180917114113.GA31460@paquier.xyz>
 <17194.1537191697@sss.pgh.pa.us>
 <20180917140202.GF31460@paquier.xyz>
 <19655.1537195712@sss.pgh.pa.us>
 <f62917d5-cc77-c6ac-847f-44f922ccdd97@2ndQuadrant.com>
 <3883.1537200836@sss.pgh.pa.us>
 <11148.1537227504@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="df+09Je9rNq3P+GE"
Content-Disposition: inline
In-Reply-To: <11148.1537227504@sss.pgh.pa.us>
Precedence: bulk


--df+09Je9rNq3P+GE
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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)
> =20
>  	if (strchr(mode, 'b'))
>  		openmode |=3D O_BINARY;
> -	if (strchr(mode, 't'))
> +	else
>  		openmode |=3D 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?vi=
ew=3Dvs-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=3Dvs-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

--df+09Je9rNq3P+GE
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlugQr8ACgkQnvQgOdby
QH31YRAAol87KybcxakNFWiKxj8r3A3TPNjMxTswfjO8it7S+QgYCZGqt0sKfTq1
pPu+NfOgVfArUtZqlfDYyxxZnknCrngZxLFScifv+PIojf2tVIc2hLoM33lL7T3j
6N7xIco4B8mNzzPvaKIqAJK5C9OmWc1zHNMJv7BPN1OSt+BO+2s7aQqaGuLcFl0j
grKoMaMFjyQxXG1ktIOZBMzeR1zzMhI461dGnloWvH7ZpKDNS3GoUkO34h/WFoDX
Ur8zE4Mnt1V99rtgzk/HwnEWzuViLplZalIOJTelIx3KkHCnjICSktOprUpjK2+j
QSGNya0rV4o1KeSWBpuaoMqewkOnkKsOdKRk4qZCSrTDlKNGF6DQdMJDpFEFp3v1
iUqVMHH+X+l478NK8wubOX7pB33HWihhe+Sdd4CNJDcEwmNcmhvZnTK4uwg4Y7R6
CU99j3k4loz4XNVHeri4ks2qbRk1ACJOSfAKtCzVrBaNjiqNehJo+aBODQpKxHA7
QJZOAAb+5fj2T6qmTo8c3O4cPInVkaIs8+KOc5canTYdBxcnoK3EZ+pNq5hHtc8H
4OS1BPKqThY7w2Sj2eB1Z55JDUwkZ7ijyMHxs0khkqNOpImHzKOHpzlNVGO8na+N
Jbs0EsNoBUvCaqRfKJYviL9o0vlxgPcBS+5DWrO9m6RF3otmPvc=
=ngjq
-----END PGP SIGNATURE-----

--df+09Je9rNq3P+GE--