Re: Loaded footgun open_datasync on Windows

On Wed, 2018-09-12 at 14:43 +0900, Michael Paquier wrote:
> 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...

Thanks for that and the rest of your review.

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

As soon as I get to our Windows machine with a bit of time on my hands,
I'll try to remove that hack in pg_ctl and see if that makes pg_upgrade
work without the WIN32 workarounds.

> +#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.

Agreed, will do.

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

I missed that since PG_FLUSH_DATA_WORKS is never defined on Windows,
but I agree that it can't hurt to use the three-argument form of
open(2) there too, just in case Windows becomes more POSIX-compliant...

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

That's the bitrot I mentioned above; looks like I attached the wrong
version of the patch. Will amend.

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

Yours,
Laurenz Albe