Re: Loaded footgun open_datasync on Windows

On Mon, Sep 10, 2018 at 04:46:40PM +0200, Laurenz Albe wrote:
> I didn't get pg_upgrade to work without the log file hacks; I suspect
> that there is more than just log file locking going on, but my Windows
> skills are limited.
>
> How shall I proceed?

I do like this patch, and we have an occasion to clean a bunch of things
in pg_upgrade, so this argument is enough to me to put my hands in the
dirt and check by myself, so...

> I think that it is important to get pg_test_fsync to work correctly on
> Windows, and if my patch does not break the buildfarm, that's what it
> does.

This argument argument is sound, still... [ ... suspense ... ]

> I have attached a new version, the previous one was bit-rotted.

I really thought that this was not ambitious enough, so I have hacked on
top of your patch, so as pg_upgrade concurrent issues are removed, and I
have found one barrier in pg_ctl which decides that it is smarter to
redirect the log file (here pg_upgrade_server.log) using CMD. The
problem is that the lock taken by the process which does the redirection
does not work nicely with what pg_upgrade does in parallel. So I think
that it is better to drop that part.

+#ifdef WIN32
+ if ((infile = fopen(path, "rt")) == NULL)
+#else
if ((infile = fopen(path, "r")) == NULL)
+#endif
This should have a comment, saying roughly that as this uses
win32_fopen, text mode needs to be enforced to get proper CRLF.

One spot for open() is missed in file_utils.c, please see
pre_sync_fname().

The patch fails to apply for pg_verify_checksums, with a conflict easy
enough to fix.

At the end I would be incline to accept the patch proposed, knowing that
this would fix https://postgr.es/m/16922.1520722108@sss.pgh.pa.us
mentioned by Thomas upthread as get_pgpid would do things in a shared
manner, putting an end at some of the random failures we've seen on the
buildfarm.

Laurenz, could you update your patch? I am switching that as waiting on
author for now.
Thanks,
--
Michael