Re: Loaded footgun open_datasync on Windows

From: Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Noah Misch <noah(at)leadboat(dot)com>, Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Loaded footgun open_datasync on Windows
Date: 2018-09-12 12:08:53
Message-ID: 34510aa4ac94526310c96ed808370160d89c98f2.camel@cybertec.at
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Jesper Pedersen 2018-09-12 12:23:24 Re: executor relation handling
Previous Message Christoph Berg 2018-09-12 11:25:48 Re: Collation versioning