Re: pg_archivecleanup bug

On 13 March 2014 05:48, Bruce Momjian <bruce@momjian.us> wrote:
> On Mon, Dec  9, 2013 at 11:27:28AM -0500, Robert Haas wrote:
>> On Thu, Dec 5, 2013 at 6:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > 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?
>>
>> If there's enough uniformity in all of those places to make that
>> feasible, it certainly seems wise to do it that way.  I don't know if
>> that's the case, though - e.g. maybe some callers want to exit and
>> others do not.  pg_resetxlog wants to exit; pg_archivecleanup and
>> pg_standby most likely want to print an error and carry on.
>
> 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.
>
> 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.

pg_resetxlog was not an offender here; its coding was sound.

We shouldn't be discussing backpatching a patch that contains changes
to coding style.

ISTM we should change the code with missing checks to adopt the coding
style of pg_resetxlog, not the other way around.

I assume you or Kevin have this in hand and you don't want me to apply
the patch? (Since it was originally my bug)

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services