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(at)momjian(dot)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 sets errno 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.c
if( dirp->dd_private.dd_stat++ > 0 )
{
/* Otherwise...
*
* Get the next search entry. POSIX mandates that this must
* return NULL after the last entry has been read, but that it
* MUST NOT change errno in this case. MS-Windows _findnext()
* DOES change errno (to ENOENT) after the last entry has been
* read, so we must be prepared to restore it to its previous
* value, when no 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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +