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 :)
> 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.
--
Michael