Re: WAL-related tools and .paritial WAL file

On Wed, Jul 1, 2015 at 5:14 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 1 July 2015 at 07:52, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>>
>> On Wed, Jul 1, 2015 at 12:39 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
>> wrote:
>> > WAL-related tools, i.e., pg_archivecleanup, pg_resetxlog and
>> > pg_xlogdump don't seem to properly handle .paritial WAL file.
>> > I think that we should fix at least pg_archivecleanup, otherwise,
>> > in the system using pg_archivecleanup to clean up old archived
>> > files, the archived .paritial WAL file will never be removed.
>> > Thought?
>>
>> +1 to handle it properly in pg_archivecleanup. If people use the
>> archive cleanup command in recovery.conf and they remove WAL files
>> before the fork point of promotion they should not see the partial
>> file as well.
>>
>>
>> > To make pg_archivecleanup detect .paritial WAL file, I'd like to
>> > use IsPartialXLogFileName() macro that commit 179cdd0 added.
>> > Fortunately Michael proposed the patch which makes the macros
>> > in xlog_internal.h usable in WAL-related tools, and it's better
>> > to apply the fix based on his patch. So my plan is to apply his
>> > patch first and then apply the fix to pg_archivecleanup.
>> >
>> > http://www.postgresql.org/message-id/CAB7nPqSiKU+8HjVe_J05btonC5E7kvmRaLpGS6EaEQe=Dy3jnQ@mail.gmail.com
>>
>> As we already use extensively XLogFromFileName in a couple of frontend
>> utilies, I suspect that the buildfarm is not going to blow up, but it
>> is certainly better to have certitude with a full buildfarm cycle
>> running on it...
>>
>> > Regarding pg_resetxlog and pg_xlogdump, do we need to change
>> > also them so that they can handle .paritial file properly?
>> > pg_resetxlog cannot clean up .partial file even if it should be
>> > removed. But the remaining .paritial file is harmless and will be
>> > recycled later by checkpoint, so extra treatment of .paritial
>> > file in pg_resetxlog may not be necessary. IOW, maybe we can
>> > document that's the limitation of pg_resetxlog.
>>
>> I would rather vote for having pg_resetxlog remove the .partial
>> segment as well. That's a two-liner in KillExistingArchiveStatus and
>> KillExistingXLOG at quick glance. It looks to me that as pg_resetxlog
>> is a reset utility, it should do its jib.
>>
>> > Also regarding pg_xlogdump, we can just document, for example,
>> > "please get rid of .paritial suffix from the WAL file name if
>> > you want to dump it by pg_xlogdump".
>>
>> For pg_xlogdump, I am on board for only documenting the limitation
>> (renaming the file by yourself) as it is after all only a debug tool.
>> This would also make fuzzy_open_file more complicated by having to
>> detect two times more potential grammars... That's a bit crazy for a
>> very narrow use-case.
>
>
> +1 to all

I also agree with Michael. I implemented the patch accordingly. Patch attached.

Regards,

--
Fujii Masao