Re: pg_archivecleanup bug

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> | Applications wishing to check for error situations should set
> | errno to 0 before calling readdir(). If errno is set to non-zero
> | on return, an error occurred.

> So an error in scanning the directory will not be reported; the
> cleanup will quietly terminate the WAL deletions without processing
> the remainder of the directory. Attached is the simplest fix,
> which would report the error, stop looking for WAL files, and
> continue with other clean-ups. I'm not sure we should keep the fix
> that simple. We could set a flag so that we would exit with a
> non-zero code, or we could try a new directory scan as long as the
> last scan found and deleted at least one WAL file. Perhaps we want
> to back-patch the simple fix and do something fancier for 9.4?

A quick grep shows about ten other readdir() usages, most of which
have a similar disease.

In general, I think there is no excuse for code in the backend to use
readdir() directly; it should be using ReadDir(), which takes care of this
as well as error reporting. It appears that src/backend/storage/ipc/dsm.c
didn't get that memo; it certainly is innocent of any error checking
concerns. But the other usages seem to be in assorted utilities, which
will need to do it right for themselves. initdb.c's walkdir() seems to
have it right and might be a reasonable model to follow. Or maybe we
should invent a frontend-friendly version of ReadDir() rather than
duplicating all the error checking code in ten-and-counting places?

regards, tom lane