Allow pg_archivecleanup to remove backup history files

At Tue, 25 Apr 2023 16:38:16 +0900, torikoshia <torikoshia@oss.nttdata.com> wrote in

Hi,

Currently pg_archivecleanup doesn't remove backup history files even
when they're older than oldestkeptwalfile.

Of course the size of backup history files are smaller than WAL files
and they wouldn't consume much disk space, but a lot of backup history
files(e.g. daily backup for years) whose backups data have been
already removed are unnecessary and I would appreciate if
pg_archivecleanup has an option to remove them.

Attached a PoC patch, which added new option -b to remove files
including backup history files older than oldestkeptwalfile.

$ ls archivedir
000000010000000000000001 000000010000000000000003
000000010000000000000006
000000010000000000000008
000000010000000000000002 000000010000000000000004
000000010000000000000007
000000010000000000000009
000000010000000000000002.00000028.backup 000000010000000000000005
000000010000000000000007.00000028.backup
00000001000000000000000A.partial

$ pg_archivecleanup -b archivedir 000000010000000000000009

$ ls archivedir
000000010000000000000009 00000001000000000000000A.partial

Any thoughts?

I thought that we have decided not to do that, but I coundn't find any
discussion about it in the ML archive. Anyway, I think it is great
that we have that option.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Apr 25, 2023 at 05:29:48PM +0900, Kyotaro Horiguchi wrote:

I thought that we have decided not to do that, but I coundn't find any
discussion about it in the ML archive. Anyway, I think it is great
that we have that option.

No objections from here to make that optional. It's been argued for a
couple of times that leaving the archive history files is good for
debugging, but I can also get why one would one to clean up all the
files older than a WAL retention policy. Even if these are just few
bytes, it can be noisy for the eye to see a large accumulation of
history files mixed with the WAL segments.
--
Michael

Horiguchi-san, Michael-san

Thanks for your comments and information!

Attached a patch with documentation and regression tests.

On 2023-04-26 06:39, Michael Paquier wrote:

On Tue, Apr 25, 2023 at 05:29:48PM +0900, Kyotaro Horiguchi wrote:

I thought that we have decided not to do that, but I coundn't find any
discussion about it in the ML archive. Anyway, I think it is great
that we have that option.

No objections from here to make that optional. It's been argued for a
couple of times that leaving the archive history files is good for
debugging, but I can also get why one would one to clean up all the
files older than a WAL retention policy. Even if these are just few
bytes, it can be noisy for the eye to see a large accumulation of
history files mixed with the WAL segments.
--
Michael

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

On Tue, May 9, 2023 at 7:03 PM torikoshia <torikoshia@oss.nttdata.com> wrote:

Attached a patch with documentation and regression tests.

Thanks. I think pg_archivecleanup cleaning up history files makes it a
complete feature as there's no need to write custom code/scripts over
and above what pg_archivecleanup provides. It will help those who are
using pg_archivecleanup for cleaning up older WAL files, say from
their archive location.

Just curious to know the driving point behind this proposal - is
pg_archivecleanup deployed in production that was unable to clean up
the history files and there were many such history files left? It will
help us know how pg_archivecleanup is being used.

I'm wondering if making -x generic with '-x' '.backup', is simpler
than adding another option?

Comments on the patch:
1. Why just only the backup history files? Why not remove the timeline
history files too? Is it because there may not be as many tli switches
happening as backups?
2.+sub remove_backuphistoryfile_run_check
+{
Why to invent a new function when run_check() can be made generic with
few arguments passed? For instance, run_check() can receive
pg_archivecleanup command args, what to use for create_files(), in the
error condition if the pg_archivecleanup command args contain 'b',
then use a different message "$test_name: first older WAL file was
cleaned up" or "$test_name: first .backup file was cleaned up".
Otherwise, just modify the messages to be:
"$test_name: first older file %s was cleaned up", $files[0]);
"$test_name: second older file %s was cleaned up", $files[1]);
"$test_name: restart file %s was not cleaned up", $files[2]);
"$test_name: newer file %s was not cleaned up", $files[3]);
"$test_name: unrelated file %s was not cleaned up", $files[4]);

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 2023-05-10 17:52, Bharath Rupireddy wrote:
Thanks for your comments!

Just curious to know the driving point behind this proposal - is
pg_archivecleanup deployed in production that was unable to clean up
the history files and there were many such history files left? It will
help us know how pg_archivecleanup is being used.

Yes.

Just curious to know the driving point behind this proposal - is
pg_archivecleanup deployed in production that was unable to clean up
the history files and there were many such history files left? It will
help us know how pg_archivecleanup is being used.

I'm wondering if making -x generic with '-x' '.backup', is simpler
than adding another option?

Since according to the current semantics, deleting backup history files
with -x demands not '-x .backup' but '-x .007C9330.backup' when the file
name is 0000000100001234000055CD.007C9330.backup, it needs special
treatment for backup history files, right?

I think it would be intuitive and easier to remember than new option.

I was a little concerned about what to do when deleting both the files
ending in .gz and backup history files.
Is making it possible to specify both "-x .backup" and "-x .gz" the way
to go?

I also concerned someone might add ".backup" to WAL files, but does that
usually not happen?

Comments on the patch:
1. Why just only the backup history files? Why not remove the timeline
history files too? Is it because there may not be as many tli switches
happening as backups?

Yeah, do you think we should also add logic for '-x .history'?

2.+sub remove_backuphistoryfile_run_check
+{
Why to invent a new function when run_check() can be made generic with
few arguments passed?

Thanks, I'm going to reconsider it.

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

On Fri, May 12, 2023 at 05:53:45PM +0900, torikoshia wrote:

On 2023-05-10 17:52, Bharath Rupireddy wrote:
I was a little concerned about what to do when deleting both the files
ending in .gz and backup history files.
Is making it possible to specify both "-x .backup" and "-x .gz" the way to
go?

I also concerned someone might add ".backup" to WAL files, but does that
usually not happen?

Depends on the archive command, of course. I have seen code using
this suffix for some segment names in the past, FWIW.

Comments on the patch:
1. Why just only the backup history files? Why not remove the timeline
history files too? Is it because there may not be as many tli switches
happening as backups?

Yeah, do you think we should also add logic for '-x .history'?

Timeline history files can be critical pieces when it comes to
assigning a TLI as these could be retrieved by a restore_command
during recovery for a TLI jump or just assign a new TLI number at the
end of recovery, still you could presumably remove the TLI history
files that are older than the TLI the segment defined refers too.
(pg_archivecleanup has no specific logic to look at the history with
child TLIs for example, to keep it simple, and I'd rather keep it this
way). There may be an argument for making that optional, of course,
but it does not strike me as really necessary compared to the backup
history files which are just around for debugging purposes.

2.+sub remove_backuphistoryfile_run_check
+{
Why to invent a new function when run_check() can be made generic with
few arguments passed?

Thanks, I'm going to reconsider it.

+       <para>
+         Remove files including backup history files, whose suffix is <filename>.backup</filename>.
+         Note that when <replaceable>oldestkeptwalfile</replaceable> is a backup history file,
+         specified file is kept and only preceding WAL files and backup history files are removed.
+       </para>

This addition to the documentation looks unprecise to me. Backup
history files have a more complex format than just the .backup
suffix, and this is documented in backup.sgml.

How about plugging in some long options, and use something more
explicit like --clean-backup-history?

-   if ((IsXLogFileName(walfile) || IsPartialXLogFileName(walfile)) &&
+   if (((IsXLogFileName(walfile) || IsPartialXLogFileName(walfile)) ||
+           (removeBackupHistoryFile && IsBackupHistoryFileName(walfile))) &&
            strcmp(walfile + 8, exclusiveCleanupFileName + 8) < 0)

Could it be a bit cleaner to split this check in two, saving one level
of indentation on the way for its most inner loop? I would imagine
something like:
/* Check file name */
if (!IsXLogFileName(walfile) &&
!IsPartialXLogFileName(walfile))
continue;
/* Check cutoff point */
if (strcmp(walfile + 8, exclusiveCleanupFileName + 8) >= 0)
continue;
//rest of the code doing the unlinks.
--
Michael

On 2023-05-15 09:18, Michael Paquier wrote:

On Fri, May 12, 2023 at 05:53:45PM +0900, torikoshia wrote:

On 2023-05-10 17:52, Bharath Rupireddy wrote:
I was a little concerned about what to do when deleting both the files
ending in .gz and backup history files.
Is making it possible to specify both "-x .backup" and "-x .gz" the
way to
go?

I also concerned someone might add ".backup" to WAL files, but does
that
usually not happen?

Depends on the archive command, of course. I have seen code using
this suffix for some segment names in the past, FWIW.

Thanks for the information.
I'm going to stop adding special logic for "-x .backup" and add a new
option for removing backup history files.

Comments on the patch:
1. Why just only the backup history files? Why not remove the
timeline
history files too? Is it because there may not be as many tli
switches
happening as backups?

Yeah, do you think we should also add logic for '-x .history'?

Timeline history files can be critical pieces when it comes to
assigning a TLI as these could be retrieved by a restore_command
during recovery for a TLI jump or just assign a new TLI number at the
end of recovery, still you could presumably remove the TLI history
files that are older than the TLI the segment defined refers too.
(pg_archivecleanup has no specific logic to look at the history with
child TLIs for example, to keep it simple, and I'd rather keep it this
way). There may be an argument for making that optional, of course,
but it does not strike me as really necessary compared to the backup
history files which are just around for debugging purposes.

Agreed.

2.+sub remove_backuphistoryfile_run_check
+{
Why to invent a new function when run_check() can be made generic
with
few arguments passed?

Thanks, I'm going to reconsider it.

+       <para>
+         Remove files including backup history files, whose suffix is
<filename>.backup</filename>.
+         Note that when <replaceable>oldestkeptwalfile</replaceable>
is a backup history file,
+         specified file is kept and only preceding WAL files and
backup history files are removed.
+       </para>

This addition to the documentation looks unprecise to me. Backup
history files have a more complex format than just the .backup
suffix, and this is documented in backup.sgml.

I'm going to remove the explanation for the backup history files and
just add the hyperlink to the original explanation in backup.sgml.

How about plugging in some long options, and use something more
explicit like --clean-backup-history?

Agreed.

-   if ((IsXLogFileName(walfile) || IsPartialXLogFileName(walfile)) &&
+   if (((IsXLogFileName(walfile) || IsPartialXLogFileName(walfile)) ||
+           (removeBackupHistoryFile && 
IsBackupHistoryFileName(walfile))) &&
strcmp(walfile + 8, exclusiveCleanupFileName + 8) < 0)

Could it be a bit cleaner to split this check in two, saving one level
of indentation on the way for its most inner loop? I would imagine
something like:
/* Check file name */
if (!IsXLogFileName(walfile) &&
!IsPartialXLogFileName(walfile))
continue;
/* Check cutoff point */
if (strcmp(walfile + 8, exclusiveCleanupFileName + 8) >= 0)
continue;
//rest of the code doing the unlinks.
--

Thanks, that looks better.

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

On Mon, May 15, 2023 at 03:16:46PM +0900, torikoshia wrote:

On 2023-05-15 09:18, Michael Paquier wrote:

How about plugging in some long options, and use something more
explicit like --clean-backup-history?

Agreed.

If you begin to implement that, it seems to me that this should be
shaped with a first separate patch that refactors the code to use
getopt_long(), and a second patch for the proposed feature that builds
on top of it.
--
Michael

On 2023-05-15 15:22, Michael Paquier wrote:

On Mon, May 15, 2023 at 03:16:46PM +0900, torikoshia wrote:

On 2023-05-15 09:18, Michael Paquier wrote:

How about plugging in some long options, and use something more
explicit like --clean-backup-history?

Agreed.

If you begin to implement that, it seems to me that this should be
shaped with a first separate patch that refactors the code to use
getopt_long(), and a second patch for the proposed feature that builds
on top of it.

Thanks for your advice, attached patches.

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

On Mon, May 22, 2023 at 06:24:49PM +0900, torikoshia wrote:

Thanks for your advice, attached patches.

0001 looks OK, thanks!

+ Remove files including backup history file.

This could be reworded as "Remove backup history files.", I assume.

+         Note that when <replaceable>oldestkeptwalfile</replaceable> is a backup history file,
+         specified file is kept and only preceding WAL files and backup history files are removed.

The same thing is described at the bottom of the documentation, so it
does not seem necessary here.

-       printf(_("  -d, --debug               generate debug output (verbose mode)\n"));
-       printf(_("  -n, --dry-run             dry run, show the names of the files that would be removed\n"));
-       printf(_("  -V, --version             output version information, then exit\n"));
-       printf(_("  -x --strip-extension=EXT  clean up files if they have this extension\n"));
-       printf(_("  -?, --help                show this help, then exit\n"));
+       printf(_("  -d, --debug                 generate debug output (verbose mode)\n"));
+       printf(_("  -n, --dry-run               dry run, show the names of the files that would be removed\n"));
+       printf(_("  -b, --clean-backup-history  clean up files including backup history files\n"));
+       printf(_("  -V, --version               output version information, then exit\n"));
+       printf(_("  -x --strip-extension=EXT    clean up files if they have this extension\n"));
+       printf(_("  -?, --help                  show this help, then exit\n"));

Perhaps this indentation had better be adjusted in 0001, no big deal
either way.

-       ok(!-f "$tempdir/$walfiles[0]",
-               "$test_name: first older WAL file was cleaned up");
+       if (grep {$_ eq '-x.gz'} @options) {
+               ok(!-f "$tempdir/$walfiles[0]",
+                       "$test_name: first older WAL file with .gz was cleaned up");
+       } else {
+               ok(-f "$tempdir/$walfiles[0]",
+                       "$test_name: first older WAL file with .gz was not cleaned up");
+       }
[...]
-       ok(-f "$tempdir/$walfiles[2]",
-               "$test_name: restartfile was not cleaned up");
+       if (grep {$_ eq '-b'} @options) {
+               ok(!-f "$tempdir/$walfiles[2]",
+                       "$test_name: Backup history file was cleaned up");
+       } else {
+               ok(-f "$tempdir/$walfiles[2]",
+                       "$test_name: Backup history file was not cleaned up");
+       }

That's over-complicated, caused by the fact that all the tests rely on
the same list of WAL files to create, aka @walfiles defined at the
beginning of the script. Wouldn't it be cleaner to handle the fake
file creations with more than one global list, before each command
run? The list of files to be created could just be passed down as an
argument of run_check(), for example, before cleaning up the contents
for the next command.
--
Michael

On 2023-05-24 10:26, Michael Paquier wrote:

On Mon, May 22, 2023 at 06:24:49PM +0900, torikoshia wrote:

Thanks for your advice, attached patches.

0001 looks OK, thanks!

+ Remove files including backup history file.

This could be reworded as "Remove backup history files.", I assume.

+         Note that when <replaceable>oldestkeptwalfile</replaceable>
is a backup history file,
+         specified file is kept and only preceding WAL files and
backup history files are removed.

The same thing is described at the bottom of the documentation, so it
does not seem necessary here.

-       printf(_("  -d, --debug               generate debug output
(verbose mode)\n"));
-       printf(_("  -n, --dry-run             dry run, show the names
of the files that would be removed\n"));
-       printf(_("  -V, --version             output version
information, then exit\n"));
-       printf(_("  -x --strip-extension=EXT  clean up files if they
have this extension\n"));
-       printf(_("  -?, --help                show this help, then 
exit\n"));
+       printf(_("  -d, --debug                 generate debug output
(verbose mode)\n"));
+       printf(_("  -n, --dry-run               dry run, show the
names of the files that would be removed\n"));
+       printf(_("  -b, --clean-backup-history  clean up files
including backup history files\n"));
+       printf(_("  -V, --version               output version
information, then exit\n"));
+       printf(_("  -x --strip-extension=EXT    clean up files if they
have this extension\n"));
+       printf(_("  -?, --help                  show this help, then 
exit\n"));

Perhaps this indentation had better be adjusted in 0001, no big deal
either way.

-       ok(!-f "$tempdir/$walfiles[0]",
-               "$test_name: first older WAL file was cleaned up");
+       if (grep {$_ eq '-x.gz'} @options) {
+               ok(!-f "$tempdir/$walfiles[0]",
+                       "$test_name: first older WAL file with .gz was
cleaned up");
+       } else {
+               ok(-f "$tempdir/$walfiles[0]",
+                       "$test_name: first older WAL file with .gz was
not cleaned up");
+       }
[...]
-       ok(-f "$tempdir/$walfiles[2]",
-               "$test_name: restartfile was not cleaned up");
+       if (grep {$_ eq '-b'} @options) {
+               ok(!-f "$tempdir/$walfiles[2]",
+                       "$test_name: Backup history file was cleaned 
up");
+       } else {
+               ok(-f "$tempdir/$walfiles[2]",
+                       "$test_name: Backup history file was not 
cleaned up");
+       }

That's over-complicated, caused by the fact that all the tests rely on
the same list of WAL files to create, aka @walfiles defined at the
beginning of the script. Wouldn't it be cleaner to handle the fake
file creations with more than one global list, before each command
run? The list of files to be created could just be passed down as an
argument of run_check(), for example, before cleaning up the contents
for the next command.
--
Michael

Thanks for reviewing!
Updated patches according to your comment.

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

At Thu, 25 May 2023 23:51:18 +0900, torikoshia <torikoshia@oss.nttdata.com> wrote in

Updated patches according to your comment.

+ printf(_(" -x --strip-extension=EXT clean up files if they have this extension\n"));

Perhaps a comma is needed after "-x". The apparent inconsistency
between the long name and the description is perplexing. I think it
might be more suitable to reword the descrition to "ignore this
extension while identifying files for cleanup" or something along
those lines..., and then name the option as "--ignore-extension".

The patch leaves the code.

* Truncation is essentially harmless, because we skip names of
* length other than XLOG_FNAME_LEN. (In principle, one could use
* a 1000-character additional_ext and get trouble.)
*/
strlcpy(walfile, xlde->d_name, MAXPGPATH);
TrimExtension(walfile, additional_ext);

The comment is no longer correct about the file name length.

+			if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile) &&
+				(!cleanBackupHistory || !IsBackupHistoryFileName(walfile)))
+				continue;

I'm not certain about the others, but I see this a tad tricky to
understand at first glance. Here's how I would phrase it. (A nearby
comment might require a tweak if we go ahead with this change.)

if (!(IsXLogFileName(walfile) || IsPartialXLogFileName(walfile) ||
(cleanBackupHistory && IsBackupHistoryFileName(walfile))))

or

if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile) &&
!(cleanBackupHistory && IsBackupHistoryFileName(walfile)))

By the way, this patch reduces the nesting level. If we go that
direction, the following structure could be reworked similarly, and I
believe it results in a more standard structure.

if ((xldir = opendir(archiveLocation)) != NULL)
{
...
}
else
pg_fatal("could not open archive location..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, May 26, 2023 at 10:07:48AM +0900, Kyotaro Horiguchi wrote:

+			if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile) &&
+				(!cleanBackupHistory || !IsBackupHistoryFileName(walfile)))
+				continue;

I'm not certain about the others, but I see this a tad tricky to
understand at first glance. Here's how I would phrase it. (A nearby
comment might require a tweak if we go ahead with this change.)

if (!(IsXLogFileName(walfile) || IsPartialXLogFileName(walfile) ||
(cleanBackupHistory && IsBackupHistoryFileName(walfile))))
or
if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile) &&
!(cleanBackupHistory && IsBackupHistoryFileName(walfile)))

FWIW, I am OK with what's written in the patch, but it is true that
your second suggestion makes things a bit easier to read, perhaps.
--
Michael

On Thu, May 25, 2023 at 11:51:18PM +0900, torikoshia wrote:

Updated patches according to your comment.

-   ok(!-f "$tempdir/$walfiles[1]",
-       "$test_name: second older WAL file was cleaned up");
-   ok(-f "$tempdir/$walfiles[2]",
+   ok(!-f "$tempdir/@$walfiles[1]",
+       "$test_name: second older WAL/backup history file was cleaned up");
+   ok(-f "$tempdir/@$walfiles[2]",

This is still a bit confusing, because as designed the test has a
dependency on the number of elements present in the list, and the
description of the test may not refer to what's actually used (the
second element of each list is either a WAL segment or a backup
history file). I think that I would just rewrite that so as we have a
list of WAL segments in an array with the expected result associated
to each one of them. Basically, something like that:
my @wal_segments = (
{ name => "SEGMENT1", present => 0 },
{ name => "BACKUPFILE1", present => 1 },
{ name => "SEGMENT2", present => 0 });

Then the last part of run_check() would loop through all the elements
listed.
--
Michael

On 2023-05-26 10:07, Kyotaro Horiguchi wrote:
Thanks for your review!

+ printf(_(" -x --strip-extension=EXT clean up files if they have
this extension\n"));

Perhaps a comma is needed after "-x".

That's an oversight. Modified it.

The apparent inconsistency
between the long name and the description is perplexing. I think it
might be more suitable to reword the descrition to "ignore this
extension while identifying files for cleanup" or something along
those lines..., and then name the option as "--ignore-extension".

I used 'strip' since it is used in the manual as below:

| -x extension
| Provide an extension that will be stripped from all file names before
deciding if they should be deleted

I think using the same verb both in long name option and in the manual
is natural.
How about something like this?

| printf(_(" -x, --strip-extension=EXT strip this extention before
identifying files fo clean up\n"));

The patch leaves the code.

* Truncation is essentially harmless, because we skip names of
* length other than XLOG_FNAME_LEN. (In principle, one could use
* a 1000-character additional_ext and get trouble.)
*/
strlcpy(walfile, xlde->d_name, MAXPGPATH);
TrimExtension(walfile, additional_ext);

The comment is no longer correct about the file name length.

Yeah.
considering parital WAL, it would be not correct even before applying
the patch.
I modifiied the comments as below:

| * Truncation is essentially harmless, because we check the file
| * format including the length immediately after this.

+			if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile) &&
+				(!cleanBackupHistory || !IsBackupHistoryFileName(walfile)))
+				continue;

I'm not certain about the others, but I see this a tad tricky to
understand at first glance. Here's how I would phrase it. (A nearby
comment might require a tweak if we go ahead with this change.)

if (!(IsXLogFileName(walfile) || IsPartialXLogFileName(walfile) ||
(cleanBackupHistory && IsBackupHistoryFileName(walfile))))

or

if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile) &&
!(cleanBackupHistory && IsBackupHistoryFileName(walfile)))

Thanks for the suggestion, I used the second one.

By the way, this patch reduces the nesting level. If we go that
direction, the following structure could be reworked similarly, and I
believe it results in a more standard structure.

if ((xldir = opendir(archiveLocation)) != NULL)
{
...
}
else
pg_fatal("could not open archive location..

Agreed. Attached 0003 patch for this.

On 2023-05-26 14:19, Michael Paquier wrote:

On Thu, May 25, 2023 at 11:51:18PM +0900, torikoshia wrote:

Updated patches according to your comment.

-   ok(!-f "$tempdir/$walfiles[1]",
-       "$test_name: second older WAL file was cleaned up");
-   ok(-f "$tempdir/$walfiles[2]",
+   ok(!-f "$tempdir/@$walfiles[1]",
+       "$test_name: second older WAL/backup history file was cleaned 
up");
+   ok(-f "$tempdir/@$walfiles[2]",

This is still a bit confusing, because as designed the test has a
dependency on the number of elements present in the list, and the
description of the test may not refer to what's actually used (the
second element of each list is either a WAL segment or a backup
history file). I think that I would just rewrite that so as we have a
list of WAL segments in an array with the expected result associated
to each one of them. Basically, something like that:
my @wal_segments = (
{ name => "SEGMENT1", present => 0 },
{ name => "BACKUPFILE1", present => 1 },
{ name => "SEGMENT2", present => 0 });

Then the last part of run_check() would loop through all the elements
listed.

Thanks.
Update the patch according to the advice.
I also changed the parameter of run_check() from specifying extension of
oldestkeptwalfile to oldestkeptwalfile itself.

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

On 2023/05/31 10:51, torikoshia wrote:

Update the patch according to the advice.

Thanks for updating the patches! I have small comments regarding 0002 patch.

+ <para>
+ Remove backup history files.

Isn't it better to document clearly which backup history files to be removed? For example, "In addition to removing WAL files, remove backup history files with prefixes logically preceding the oldestkeptwalfile.".

printf(_(" -n, --dry-run dry run, show the names of the files that would be removed\n"));
+ printf(_(" -b, --clean-backup-history clean up files including backup history files\n"));

Shouldn't -b option be placed in alphabetical order?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Fri, Jun 09, 2023 at 12:32:15AM +0900, Fujii Masao wrote:

+ <para>
+ Remove backup history files.

Isn't it better to document clearly which backup history files to be removed? For example, "In addition to removing WAL files, remove backup history files with prefixes logically preceding the oldestkeptwalfile.".

I've written about this part at the beginning of this one, where this
sounds like a duplicated description of the Description section:
/messages/by-id/ZG1nq13v411y4TFL@paquier.xyz

printf(_(" -n, --dry-run dry run, show the names of the files that would be removed\n"));
+ printf(_(" -b, --clean-backup-history clean up files including backup history files\n"));

Shouldn't -b option be placed in alphabetical order?

+1.
--
Michael

On 2023-06-12 16:33, Michael Paquier wrote:

On Fri, Jun 09, 2023 at 12:32:15AM +0900, Fujii Masao wrote:

Thanks for reviewing!

printf(_(" -n, --dry-run dry run, show the names of
the files that would be removed\n"));
+ printf(_(" -b, --clean-backup-history clean up files including
backup history files\n"));

Shouldn't -b option be placed in alphabetical order?

+1.

Modified the place.

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

At Wed, 14 Jun 2023 00:49:39 +0900, torikoshia <torikoshia@oss.nttdata.com> wrote in

On 2023-06-12 16:33, Michael Paquier wrote:

On Fri, Jun 09, 2023 at 12:32:15AM +0900, Fujii Masao wrote:

Thanks for reviewing!

printf(_(" -n, --dry-run dry run, show the names of the files that
would be removed\n"));
+ printf(_(" -b, --clean-backup-history clean up files including
backup history files\n"));
Shouldn't -b option be placed in alphabetical order?

+1.

Modified the place.

-	printf(_("  -d             generate debug output (verbose mode)\n"));
-	printf(_("  -n             dry run, show the names of the files that would be removed\n"));
-	printf(_("  -V, --version  output version information, then exit\n"));
-	printf(_("  -x EXT         clean up files if they have this extension\n"));
-	printf(_("  -?, --help     show this help, then exit\n"));
+	printf(_("  -d, --debug                 generate debug output (verbose mode)\n"));
+	printf(_("  -n, --dry-run               dry run, show the names of the files that would be removed\n"));
+	printf(_("  -V, --version               output version information, then exit\n"));
+	printf(_("  -x, --strip-extension=EXT   strip this extention before identifying files fo clean up\n"));
+	printf(_("  -?, --help                  show this help, then exit\n"));

After this change, some of these lines corss the boundary of the 80
columns width. (is that policy viable noadays? I am usually working
using terminal windows with such a width..) It's somewhat unrelated to
this patch, but a help line a few lines further down also exceeds the
width. We could shorten it by removing the "/mnt/server" portion, but
I'm not sure if it's worth doing.

Or for use as a standalone archive cleaner:
e.g.
pg_archivecleanup /mnt/server/archiverdir 000000010000000000000010.00000020.backup

+ printf(_(" -x, --strip-extension=EXT strip this extention before identifying files fo clean up\n"));

s/fo/for/ ?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center