On Fri, Dec 14, 2018 at 08:43:20AM -0500, David Steele wrote:
> On 12/13/18 7:15 PM, Michael Paquier wrote:
>> I would like to hear opinion from other though if we should consider
>> that as an improvement or an actual bug fix. Changing the order of the
>> files to map with what the startup process does when promoting does not
>> sound like a bug fix to me, still this is not really invasive, so we
>> could really consider it worth back-patching to reduce common pain from
>> users when it comes to timeline handling.
>
> I think an argument can be made that it is a bug (ish). Postgres
> generates the files in one order, and they get archived in a different
> order.
I am not completely sure either. In my experience, if there is any
doubt on such definitions the best answer is to not backpatch.
>> Or you could just use IsTLHistoryFileName here?
>
> We'd have to truncate .ready off the string to make that work, which
> seems easy enough. Is that what you were thinking?
Yes, that's the idea. pgarch_readyXlog returns the segment name which
should be archived, so you could just compute it after detecting a
.ready file.
> One thing to consider is the check above is more efficient than
> IsTLHistoryFileName() and it potentially gets run a lot.
This check misses strspn(), so any file finishing with .history would
get eaten even if that's unlikely to happen.
>> If one wants to simply check this code, you can just create dummy orphan
>> files in archive_status and see in which order they get removed.
>
> Seems awfully racy. Are there currently any tests like this for the
> archiver that I can look at extending?
Sorry for the confusion, I was referring to manual testing here.
Thinking about it, we could have an automatic test to check for the file
order pattern by creating dummy files, starting the server with archiver
enabled, and then parse the logs as orphan .ready files would get
removed in the order their archiving is attempted with one WARNING entry
generated for each. I am not sure if that is worth a test though.
--
Michael
