Re: Use durable_unlink for .ready and .done files for WAL segment removal

On 12/4/18, 7:35 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Tue, Dec 04, 2018 at 08:40:40PM +0000, Bossart, Nathan wrote:
>> Thanks for the updated patch!  The code looks good to me, the patch
>> applies cleanly and builds without warnings, and it seems to work well
>> in my manual tests.  I just have a few wording suggestions.
>
> How are you testing this?  I just stop the server and manually touch
> some fake status files in archive_status :)

That's almost exactly what I was doing, too.

>> I would phrase this comment this way:
>> 
>>         Since archive_status files are not durably removed, a system
>>         crash could leave behind .ready files for WAL segments that
>>         have already been recycled or removed.  In this case, simply
>>         remove the orphan status file and move on.
>
> Fine for me.  Thanks!
>
>> +     ereport(WARNING,
>> +             (errmsg("removed orphan archive status file %s",
>> +                     xlogready)));
>> 
>> I think we should put quotes around the file name like we do elsewhere
>> in pgarch_ArchiverCopyLoop().
>
> Done.
>
>> +					ereport(WARNING,
>> +							(errmsg("failed removal of \"%s\" too many times, will try again later",
>> +									xlogready)));
>> 
>> I'd suggest mirroring the log statement for failed archiving commands
>> and saying something like, "removing orphan archive status file \"%s\"
>> failed too many times, will try again later."  IMO that makes it
>> clearer what is failing and why we are removing it in the first place.
>
> "removal of" is more consistent here I think, so changed this way with
> your wording merged.

The v4 patch looks good to me!

Nathan