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

Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> writes:

> +#ifndef WIN32
> if ((src_fd = open(fromfile, O_RDONLY, 0)) < 0)
> +#else
> + if ((src_fd = open(fromfile, O_RDONLY | O_BINARY)) < 0)
> +#endif

This is easier with PG_BINARY. Also, both open() calls need this.

I'm rather inclined to also stick PG_BINARY into the open() calls in
copy_file() as well. That function isn't actively broken since it's
not used under Windows, but I think it's highly likely that the
current bug arose from copying-and-pasting that code, so leaving it
in a state that's unsafe for Windows is just asking for trouble.

Attached is the patch I'm currently working with to fix all three
of the portability issues here (PG_BINARY, endianness, alignment,
and there's some cosmetic changes too).

> + pg_log(PG_WARNING,
> + "could not read expected bytes: read = %u, expected = %u\n",
> + BLCKSZ, bytesRead);

I think this is probably better done as part of a wholesale revision
of the error reporting in this file. What I have in mind is to
redefine copyFile, linkFile, and rewriteVisibilityMap as all being
responsible for reporting their own errors and then exiting (so
they can just return void). We'd need to pass in the schema name
and table name so that they can include that context, so that the
reports don't get any less complete than they were before, but that
does not seem like a big downside.

A variant would be to have them print the error messages but then
return a bool success flag, leaving the caller to decide whether
it's fatal or not. But that would be more complicated and I see
no real reason to think pg_upgrade would ever need it.

regards, tom lane