Re: BUG #13774: upgrade from 9.1 to 9.4 'succeeds' without enough disk space

On Tue, Nov 17, 2015 at 11:43:34PM -0500, Noah Misch wrote:
> On Fri, Nov 13, 2015 at 08:24:02PM -0500, Bruce Momjian wrote:
> > Thank you for the report.  This is embarrassing, but the code was
> > testing for the wrong return value on Windows.  We used a macro to
> > define the same symbol on Windows and Unix (pg_copy_file)
>
> > *** a/src/bin/pg_upgrade/file.c
> > --- b/src/bin/pg_upgrade/file.c
> > *************** copyAndUpdateFile(pageCnvCtx *pageConver
> > *** 34,40 ****
> >   {
> >       if (pageConverter == NULL)
> >       {
> > !         if (pg_copy_file(src, dst, force) == -1)
> >               return getErrorText(errno);
> >           else
> >               return NULL;
> > --- 34,44 ----
> >   {
> >       if (pageConverter == NULL)
> >       {
> > ! #ifndef WIN32
> > !         if (copy_file(src, dst, force) == -1)
> > ! #else
> > !         if (CopyFile(src, dst, force) == 0)
> > ! #endif
> >               return getErrorText(errno);
>
> Thanks.  This passage of code has at least two other problems on Windows.  The
> third CopyFile() argument means the opposite of the third copy_file()
> argument.  Next, getErrorText() is wrong on Windows:
>
> const char *
> getErrorText(int errNum)
> {
> #ifdef WIN32
>     _dosmaperr(GetLastError());
> #endif
>     return pg_strdup(strerror(errNum));
> }
>
> Calling _dosmaperr() to set errno is futile, because we nonetheless proceed to
> use the passed-in errNum.

Very good points.  The attached patch fixes the copy parameter and
addresses the errno issue by just using errno direcdtly after the
_dosmaperr call.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription                             +