Re: pg_archivecleanup bug

On Tue, Mar 18, 2014 at 11:25:46AM +0530, Amit Kapila wrote:
> On Thu, Mar 13, 2014 at 11:18 AM, Bruce Momjian <bruce@momjian.us> wrote:
> >
> > I have developed the attached patch which fixes all cases where
> > readdir() wasn't checking for errno, and cleaned up the syntax in other
> > cases to be consistent.
> 
> 
> 1. One common thing missed wherever handling for errno is added
> is below check which is present in all existing cases where errno
> is used (initdb.c, pg_resetxlog.c, ReadDir, ..)
> 
> #ifdef WIN32
> /*
> * This fix is in mingw cvs (runtime/mingwex/dirent.c rev 1.4), but not in
> * released version
> */
> if (GetLastError() == ERROR_NO_MORE_FILES)
> errno = 0;
> #endif

Very good point.  I have modified the patch to add this block in all
cases where it was missing.  I started to wonder about the comment and
if the Mingw fix was released.  Based on some research, I see this as
fixed in mingw-runtime-3.2, released 2003-10-10.  That's pretty old. 
(What I don't know is when that was paired with Msys in a bundled
release.)  Here is the Mingw fixed code:
http://ftp.ntua.gr/mirror/mingw/OldFiles/mingw-runtime-3.2-src.tar.gz    {      /* Get the next search entry. */
if(_tfindnext (dirp->dd_handle, &(dirp->dd_dta)))    {      /* We are off the end or otherwise error.         _findnext
setserrno to ENOENT if no more file         Undo this. */      DWORD winerr = GetLastError();      if (winerr ==
ERROR_NO_MORE_FILES)       errno = 0;
 

The current code has a better explanation:
http://sourceforge.net/p/mingw/mingw-org-wsl/ci/master/tree/src/libcrt/tchar/dirent.cif( dirp->dd_private.dd_stat++ > 0
){   /* Otherwise...    *    * Get the next search entry. POSIX mandates that this must    * return NULL after the last
entryhas been read, but that it    * MUST NOT change errno in this case. MS-Windows _findnext()    * DOES change errno
(toENOENT) after the last entry has been    * read, so we must be prepared to restore it to its previous    * value,
whenno actual error has occurred.    */    int prev_errno = errno;    if( DIRENT_UPDATE( dirp->dd_private ) != 0 )    {
      /* May be an error, or just the case described above...        */        if( GetLastError() ==
ERROR_NO_MORE_FILES)        /*        * ...which requires us to reset errno.        */        errno = prev_errno;
 

but it is basically doing the same thing.  I am wondering if we should
back-patch the PG code block where it was missing, and remove it from
head in all places on the logic that everyone running 9.4 will have a
post-3.1 version of Mingw.  Postgres 8.4 was released in 2009 and it is
possible some people are still using pre-3.2 Mingw versions with that PG
release.

> 2.
> ! if (errno || closedir(chkdir) == -1)
>   result = -1; /* some kind of I/O error? */
> 
> Is there a special need to check return value of closedir in this
> function, as all other uses (initdb.c, pg_resetxlog.c, pgfnames.c)
> of it in similar context doesn't check the same?
> 
> One thing I think for which this code needs change is to check
> errno before closedir as is done in initdb.c or pg_resetxlog.c

Yes, good point.  Patch adjusted to add this.

> > While I am not a fan of backpatching, the fact we are ignoring errors in
> > some critical cases seems the non-cosmetic parts should be backpatched.
> 
> +1

The larger the patch gets, the more worried I am about backpatching.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +