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