Subject: Allow pg_archivecleanup to remove backup history files

From: torikoshia <[email-masked]>
Date: Tue, 25 Apr 2023 16:38:16 +0900

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?

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

From ad87224ec5ba6ee13ccf934bf3e5adefb7e67212 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email-masked]>
Date: Mon, 24 Apr 2023 23:28:06 +0900
Subject: [PATCH v1] Allow pg_archivecleanup to remove backup history files

Add new option -b to remove files including backup history files older
than oldestkeptwalfile.

---
 src/bin/pg_archivecleanup/pg_archivecleanup.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index 7726d05149..5bc90fbadd 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -23,6 +23,8 @@ const char *progname;
 
 /* Options and defaults */
 bool		dryrun = false;		/* are we performing a dry-run operation? */
+bool		removeBackupHistoryFile = false;	/* remove files including
+												 * backup history files */
 char	   *additional_ext = NULL;	/* Extension to remove from filenames */
 
 char	   *archiveLocation;	/* where to find the archive? */
@@ -118,7 +120,8 @@ CleanupPriorWALFiles(void)
 			 * file. Note that this means files are not removed in the order
 			 * they were originally written, in case this worries you.
 			 */
-			if ((IsXLogFileName(walfile) || IsPartialXLogFileName(walfile)) &&
+			if (((IsXLogFileName(walfile) || IsPartialXLogFileName(walfile)) ||
+				(removeBackupHistoryFile && IsBackupHistoryFileName(walfile))) &&
 				strcmp(walfile + 8, exclusiveCleanupFileName + 8) < 0)
 			{
 				char		WALFilePath[MAXPGPATH * 2]; /* the file path
@@ -252,6 +255,7 @@ usage(void)
 	printf(_("Usage:\n"));
 	printf(_("  %s [OPTION]... ARCHIVELOCATION OLDESTKEPTWALFILE\n"), progname);
 	printf(_("\nOptions:\n"));
+	printf(_("  -b             remove files including backup history files\n"));
 	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"));
@@ -294,10 +298,13 @@ main(int argc, char **argv)
 		}
 	}
 
-	while ((c = getopt(argc, argv, "dnx:")) != -1)
+	while ((c = getopt(argc, argv, "bdnx:")) != -1)
 	{
 		switch (c)
 		{
+			case 'b':			/* Remove backup history files too */
+				removeBackupHistoryFile = true;
+				break;
 			case 'd':			/* Debug mode */
 				pg_logging_increase_verbosity();
 				break;

base-commit: c5e4ec293ea527394fc1d0006ab88047b3ce580f
-- 
2.25.1

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 25 Apr 2023 17:29:48 +0900 (JST)

At Tue, 25 Apr 2023 16:38:16 +0900, torikoshia <[email-masked]> wrote in 
> 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.
> 

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

---

From: Michael Paquier <[email-masked]>
Date: Wed, 26 Apr 2023 06:39:21 +0900

> 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

---

From: torikoshia <[email-masked]>
Date: Tue, 09 May 2023 22:32:59 +0900

Horiguchi-san, Michael-san

Thanks for your comments and information!

Attached a patch with documentation and regression tests.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

From 7d44134e5de930ce04819285a5b7359e370708d4 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <[email-masked]>
Date: Tue, 9 May 2023 21:52:01 +0900
Subject: [PATCH v2] Allow pg_archivecleanup to remove backup history files

---
 doc/src/sgml/ref/pgarchivecleanup.sgml        | 11 +++++
 src/bin/pg_archivecleanup/pg_archivecleanup.c | 11 ++++-
 .../t/010_pg_archivecleanup.pl                | 45 +++++++++++++++++--
 3 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/ref/pgarchivecleanup.sgml b/doc/src/sgml/ref/pgarchivecleanup.sgml
index 635e7c7685..6cb565156f 100644
--- a/doc/src/sgml/ref/pgarchivecleanup.sgml
+++ b/doc/src/sgml/ref/pgarchivecleanup.sgml
@@ -93,6 +93,17 @@ pg_archivecleanup:  removing file "archive/00000001000000370000000E"
 
     <variablelist>
 
+     <varlistentry>
+      <term><option>-b</option></term>
+      <listitem>
+       <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>
+      </listitem>
+     </varlistentry>
+
      <varlistentry>
       <term><option>-d</option></term>
       <listitem>
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index 7726d05149..ae6ae76f08 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -23,6 +23,8 @@ const char *progname;

 
-	while ((c = getopt(argc, argv, "dnx:")) != -1)
+	while ((c = getopt(argc, argv, "bdnx:")) != -1)
 	{
 		switch (c)
 		{
+			case 'b': 			/* Remove backup history files too */
+				removeBackupHistoryFile = true;
+				break;
 			case 'd':			/* Debug mode */
 				pg_logging_increase_verbosity();
 				break;
diff --git a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
index 76321d1284..afd2ff6209 100644
--- a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
+++ b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
@@ -16,9 +16,14 @@ my @walfiles = (
 	'00000001000000370000000C.gz', '00000001000000370000000D',
 	'00000001000000370000000E',    '00000001000000370000000F.partial',);
 
+my @walfiles_with_backuphistoryfile = (
+	'000000020000003800000007.00000028.backup', '000000020000003800000008',
+	'000000020000003800000009',
+	'00000002000000380000000A', '00000002000000380000000B.007C9330.backup');
+
 sub create_files
 {
-	foreach my $fn (@walfiles, 'unrelated_file')
+	foreach my $fn (@_, 'unrelated_file')
 	{
 		open my $file, '>', "$tempdir/$fn";
 		print $file 'CONTENT';
@@ -27,7 +32,7 @@ sub create_files
 	return;
 }
 
-create_files();
+create_files(@walfiles);
 
 command_fails_like(
 	['pg_archivecleanup'],
@@ -76,7 +81,7 @@ sub run_check
 
 	my ($suffix, $test_name) = @_;
 
-	create_files();
+	create_files(@walfiles);
 
 	command_ok(
 		[
@@ -98,8 +103,42 @@ sub run_check
 	return;
 }
 
+sub remove_backuphistoryfile_run_check
+{
+	local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+	my ($suffix, $test_name) = @_;
+
+	create_files(@walfiles_with_backuphistoryfile);
+
+	command_ok(
+		[
+			'pg_archivecleanup', '-b', $tempdir,
+			$walfiles_with_backuphistoryfile[2] . $suffix
+		],
+		"$test_name: remove_backuphistoryfile_runs");
+
+	ok(!-f "$tempdir/$walfiles_with_backuphistoryfile[0]",
+		"$test_name: first .backup file was cleaned up");
+	ok(!-f "$tempdir/$walfiles_with_backuphistoryfile[1]",
+		"$test_name: second older WAL file was cleaned up");
+	ok(-f "$tempdir/$walfiles_with_backuphistoryfile[2]",
+		"$test_name: restartfile was not cleaned up");
+	ok(-f "$tempdir/$walfiles_with_backuphistoryfile[3]",
+		"$test_name: newer WAL file was not cleaned up");
+	ok(-f "$tempdir/$walfiles_with_backuphistoryfile[4]",
+		"$test_name: newer .backup file was not cleaned up");
+	ok(-f "$tempdir/unrelated_file",
+		"$test_name: unrelated file was not cleaned up");
+	return;
+}
+
 run_check('',                 'pg_archivecleanup');
 run_check('.partial',         'pg_archivecleanup with .partial file');
 run_check('.00000020.backup', 'pg_archivecleanup with .backup file');
 
+remove_backuphistoryfile_run_check('',				   'pg_archivecleanup -b');
+remove_backuphistoryfile_run_check('.00000020.backup',
+									'pg_archivecleanup -b with .backup file');
+
 done_testing();

base-commit: c5b7f67fcc8c4a01c82660eb0996a3c697fac283
-- 
2.25.1

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 10 May 2023 14:22:02 +0530

> 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