Re: [HACKERS] pg_upgrade from 9.5 to 9.6 fails with "invalid argument"

Tom Lane wrote:
> Thomas Kellerer <spam_eater(at)gmx(dot)net> writes:
> > for some reason pg_upgrade failed on Windows 10 for me, with an error message that one specifc _vm file couldn't be copied.
>
> Hmm ... a _vm file would go through rewriteVisibilityMap(), which is new
> code for 9.6 and hasn't really gotten that much testing. Its error
> reporting is shamefully bad --- you can't tell which step failed, and
> I wouldn't even put a lot of faith in the errno being meaningful,
> considering that it does close() calls before capturing the errno.

So we do close() in a bunch of places while closing shop, which calls
_close() on Windows; this function sets errno. Then we call
getErrorText(), which calls _dosmaperr() on the result of
GetLastError(). But the last-error stuff is not set by _close; I suppose
GetLastError() returns 0 in that case, which promps _doserrmap to set errno to 0.
http://stackoverflow.com/questions/20056851/getlasterror-errno-formatmessagea-and-strerror-s
So this wouldn't quite have the effect you say; I think it'd say
"Failure while copying ...: Success" instead.

However surely we should have errno save/restore.

Other than that, I think the _dosmaperr() call should go entirely.
Moreover I think getErrorText() as a whole is misconceived and should be
removed altogether (why pstrdup the string?). There are very few places
in pg_upgrade that require _dosmaperr; I can see only copyFile and
linkFile. All the others should just be doing strerror() only, at least
according to the manual.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services