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