Обсуждение: WAL-related tools and .paritial WAL file

Hi,

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?

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

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.

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".

Thought?

Regards,

-- 
Fujii Masao

On Wed, Jul 1, 2015 at 12:39 PM, Fujii Masao <masao.fujii@gmail.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.
-- 
Michael

On Wed, Jul 1, 2015 at 2:52 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Jul 1, 2015 at 9:09 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> 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".
>>
>
> Can't we skip such files in pg_xlogdump?

.partial files are at the end of a timeline, so they will be ignored
now (and this even if a node on the old timeline archives the same
segment as the .partial one sent by the promoted standby). And as
pg_xlogdump is not able to follow a timeline jump there is nothing we
would need to do:
$ pg_xlogdump 000000010000000000000006 000000020000000000000008
pg_xlogdump: FATAL:  could not find file "000000020000000000000006":
No such file or directory
$ pg_xlogdump -t 1 000000010000000000000006 000000020000000000000008
pg_xlogdump: FATAL:  could not find file "000000020000000000000006":
No such file or directory
I am not convinced that it is worth to make pg_xlogdump follow
timeline jumps as well.. One could just run it twice on the old and
new timeline segments to get the output he wants.
-- 
Michael

On Wed, Jul 1, 2015 at 5:14 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 1 July 2015 at 07:52, Michael Paquier <michael.paquier@gmail.com> wrote:
>>
>> On Wed, Jul 1, 2015 at 12:39 PM, Fujii Masao <masao.fujii@gmail.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

On Thu, Jul 2, 2015 at 2:06 PM, Fujii Masao wrote:
> I implemented the patch accordingly. Patch attached.

Cool, thanks.

I have done some tests with pg_archivecleanup, and it works as
expected, aka if I define a backup file, the backup file as well as
the segments equal or newer than it remain. It works as well when
defining a .partial file. I have done as well some testing with
pg_resetxlog and the partial segment gets removed.

Here are some comments:
1) pgarchivecleanup.sgml needs to be updated:  In this mode, if you specify a <filename>.backup</> file name, then
only the file prefix
Here we should mention that it is also the case of a .partial file.
2)
-        * restartWALFileName is a .backup filename, make sure we use the prefix
-        * of the filename, otherwise we will remove wrong files since
+        * restartWALFileName is a .partial or .backup filename, make
sure we use
+        * the prefix of the filename, otherwise we will remove wrong
files since        * 000000010000000000000010.00000020.backup is after        * 000000010000000000000010.
Shouldn't this be made clearer as well regarding .partial files? For
example with a comment like that:
otherwise we will remove wrong files since
000000010000000000000010.00000020.backup or
000000010000000000000010.00000020.partial are after
000000010000000000000010. Simply not mentioning those file names
directly is fine for me.
3) Something not caused by this patch that I just noticed... But
pg_resetxlog does not remove .backup files in pg_xlog. Shouldn't they
get moved away as well?
Regards,
-- 
Michael

On Thu, Jul 2, 2015 at 3:48 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Jul 2, 2015 at 2:06 PM, Fujii Masao wrote:
>> I implemented the patch accordingly. Patch attached.
>
> Cool, thanks.
>
> I have done some tests with pg_archivecleanup, and it works as
> expected, aka if I define a backup file, the backup file as well as
> the segments equal or newer than it remain. It works as well when
> defining a .partial file. I have done as well some testing with
> pg_resetxlog and the partial segment gets removed.

Thanks for testing the patch!

Attached is the updated version of the patch.

> Here are some comments:
> 1) pgarchivecleanup.sgml needs to be updated:
>    In this mode, if you specify a <filename>.backup</> file name, then
> only the file prefix
> Here we should mention that it is also the case of a .partial file.

Applied.

> 2)
> -        * restartWALFileName is a .backup filename, make sure we use the prefix
> -        * of the filename, otherwise we will remove wrong files since
> +        * restartWALFileName is a .partial or .backup filename, make
> sure we use
> +        * the prefix of the filename, otherwise we will remove wrong
> files since
>          * 000000010000000000000010.00000020.backup is after
>          * 000000010000000000000010.
> Shouldn't this be made clearer as well regarding .partial files? For
> example with a comment like that:
> otherwise we will remove wrong files since
> 000000010000000000000010.00000020.backup or
> 000000010000000000000010.00000020.partial are after
> 000000010000000000000010. Simply not mentioning those file names
> directly is fine for me.

Applied.

> 3) Something not caused by this patch that I just noticed... But
> pg_resetxlog does not remove .backup files in pg_xlog. Shouldn't they
> get moved away as well?

pg_resetxlog doesn't remove also .history file in pg_xlog. Those remaining
.backup and .history files are harmless after pg_resetxlog is executed.
So I don't think that it's required to remove them. Of course we can do that,
but some existing applications might depend on the current behavior...
So unless there is strong reason to do that, I'd like to let it as it is.

Regards,

--
Fujii Masao

On Thu, Jul 2, 2015 at 8:42 PM, Fujii Masao wrote:
>> 3) Something not caused by this patch that I just noticed... But
>> pg_resetxlog does not remove .backup files in pg_xlog. Shouldn't they
>> get moved away as well?
>
> pg_resetxlog doesn't remove also .history file in pg_xlog. Those remaining
> .backup and .history files are harmless after pg_resetxlog is executed.
> So I don't think that it's required to remove them. Of course we can do that,
> but some existing applications might depend on the current behavior...
> So unless there is strong reason to do that, I'd like to let it as it is.

Well, I was just surprised to not see them wiped out. Let's not change
the behavior then that exists for ages. The rest of the patch looks
fine to me (I would add dots at the end of sentences in comment
blocks, but that's a detail). The new macros really make the code
easier to read and understand!
-- 
Michael

On Thu, Jul 2, 2015 at 8:59 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Jul 2, 2015 at 8:42 PM, Fujii Masao wrote:
>>> 3) Something not caused by this patch that I just noticed... But
>>> pg_resetxlog does not remove .backup files in pg_xlog. Shouldn't they
>>> get moved away as well?
>>
>> pg_resetxlog doesn't remove also .history file in pg_xlog. Those remaining
>> .backup and .history files are harmless after pg_resetxlog is executed.
>> So I don't think that it's required to remove them. Of course we can do that,
>> but some existing applications might depend on the current behavior...
>> So unless there is strong reason to do that, I'd like to let it as it is.
>
> Well, I was just surprised to not see them wiped out. Let's not change
> the behavior then that exists for ages. The rest of the patch looks
> fine to me (I would add dots at the end of sentences in comment
> blocks, but that's a detail). The new macros really make the code
> easier to read and understand!

Yep!

Applied the patch. Thanks!

Regards,

-- 
Fujii Masao