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
From 024b54d5d436bb88bb80bf6b316f697eb96b53f1 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikos...@oss.nttdata.com>
Date: Thu, 25 May 2023 23:13:12 +0900
Subject: [PATCH v4 1/2] Introduce pg_archivecleanup into getopt_long
This patch is a preliminary step to add an easy-to-understand option
to delete backup history files, but it also adds long options to
the existing options.
---
doc/src/sgml/ref/pgarchivecleanup.sgml | 5 ++++-
src/bin/pg_archivecleanup/pg_archivecleanup.c | 20 ++++++++++++-------
2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/ref/pgarchivecleanup.sgml b/doc/src/sgml/ref/pgarchivecleanup.sgml
index 635e7c7685..09991c2fcd 100644
--- a/doc/src/sgml/ref/pgarchivecleanup.sgml
+++ b/doc/src/sgml/ref/pgarchivecleanup.sgml
@@ -95,6 +95,7 @@ pg_archivecleanup: removing file "archive/00000001000000370000000E"
<varlistentry>
<term><option>-d</option></term>
+ <term><option>--debug</option></term>
<listitem>
<para>
Print lots of debug logging output on <filename>stderr</filename>.
@@ -104,6 +105,7 @@ pg_archivecleanup: removing file "archive/00000001000000370000000E"
<varlistentry>
<term><option>-n</option></term>
+ <term><option>--dry-run</option></term>
<listitem>
<para>
Print the names of the files that would have been removed on <filename>stdout</filename> (performs a dry run).
@@ -122,7 +124,8 @@ pg_archivecleanup: removing file "archive/00000001000000370000000E"
</varlistentry>
<varlistentry>
- <term><option>-x</option> <replaceable>extension</replaceable></term>
+ <term><option>-x <replaceable class="parameter">extension</replaceable></option></term>
+ <term><option>--strip-extension=<replaceable class="parameter">extension</replaceable></option></term>
<listitem>
<para>
Provide an extension
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index 7726d05149..707d7d54cf 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -17,7 +17,7 @@
#include "access/xlog_internal.h"
#include "common/logging.h"
-#include "pg_getopt.h"
+#include "getopt_long.h"
const char *progname;
@@ -252,11 +252,11 @@ usage(void)
printf(_("Usage:\n"));
printf(_(" %s [OPTION]... ARCHIVELOCATION OLDESTKEPTWALFILE\n"), progname);
printf(_("\nOptions:\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"));
- 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 clean up files if they have this extension\n"));
+ printf(_(" -?, --help show this help, then exit\n"));
printf(_("\n"
"For use as archive_cleanup_command in postgresql.conf:\n"
" archive_cleanup_command = 'pg_archivecleanup [OPTION]... ARCHIVELOCATION %%r'\n"
@@ -274,6 +274,12 @@ usage(void)
int
main(int argc, char **argv)
{
+ static struct option long_options[] = {
+ {"debug", no_argument, NULL, 'd'},
+ {"dry-run", no_argument, NULL, 'n'},
+ {"strip-extension", required_argument, NULL, 'x'},
+ {NULL, 0, NULL, 0}
+ };
int c;
pg_logging_init(argv[0]);
@@ -294,7 +300,7 @@ main(int argc, char **argv)
}
}
- while ((c = getopt(argc, argv, "dnx:")) != -1)
+ while ((c = getopt_long(argc, argv, "dnx:", long_options, NULL)) != -1)
{
switch (c)
{
--
2.39.2
From 23d20f30eb0fa6daed1f5e631c5b3bedaeae623d Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikos...@oss.nttdata.com>
Date: Thu, 25 May 2023 23:25:28 +0900
Subject: [PATCH v4 2/2] Allow pg_archivecleanup to remove backup history files
Backup history files are just few bytes, but it can be noisy for the
eye to see a large accumulation of history files mixed with the WAL
segments.
This patch adds a new option to remove files including backup
history files older oldestkeptwalfile.
---
doc/src/sgml/ref/pgarchivecleanup.sgml | 11 +++
src/bin/pg_archivecleanup/pg_archivecleanup.c | 78 ++++++++++++-------
.../t/010_pg_archivecleanup.pl | 46 ++++++-----
3 files changed, 87 insertions(+), 48 deletions(-)
diff --git a/doc/src/sgml/ref/pgarchivecleanup.sgml b/doc/src/sgml/ref/pgarchivecleanup.sgml
index 09991c2fcd..8fac233db6 100644
--- a/doc/src/sgml/ref/pgarchivecleanup.sgml
+++ b/doc/src/sgml/ref/pgarchivecleanup.sgml
@@ -113,6 +113,17 @@ pg_archivecleanup: removing file "archive/00000001000000370000000E"
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-b</option></term>
+ <term><option>--clean-backup-history</option></term>
+ <listitem>
+ <para>
+ Remove backup history files.
+ For details about backup history file, please refer to the <xref linkend="backup-base-backup"/>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index 707d7d54cf..3fd4a441b2 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 cleanBackupHistory = false; /* remove files including
+ * backup history files */
char *additional_ext = NULL; /* Extension to remove from filenames */
char *archiveLocation; /* where to find the archive? */
@@ -97,6 +99,8 @@ CleanupPriorWALFiles(void)
{
while (errno = 0, (xlde = readdir(xldir)) != NULL)
{
+ char WALFilePath[MAXPGPATH * 2]; /* the file path
+ * including archive */
/*
* Truncation is essentially harmless, because we skip names of
* length other than XLOG_FNAME_LEN. (In principle, one could use
@@ -106,6 +110,19 @@ CleanupPriorWALFiles(void)
TrimExtension(walfile, additional_ext);
/*
+ * Check file name.
+ *
+ * We skip files which are not WAL file or partial WAL file.
+ * Also we skip backup history files when --clean-backup-history
+ * is not specified.
+ */
+ if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile) &&
+ (!cleanBackupHistory || !IsBackupHistoryFileName(walfile)))
+ continue;
+
+ /*
+ * Check cutoff point.
+ *
* We ignore the timeline part of the XLOG segment identifiers in
* deciding whether a segment is still needed. This ensures that
* we won't prematurely remove a segment from a parent timeline.
@@ -118,39 +135,35 @@ 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)) &&
- strcmp(walfile + 8, exclusiveCleanupFileName + 8) < 0)
- {
- char WALFilePath[MAXPGPATH * 2]; /* the file path
- * including archive */
+ if (strcmp(walfile + 8, exclusiveCleanupFileName + 8) >= 0)
+ continue;
+ /*
+ * Use the original file name again now, including any
+ * extension that might have been chopped off before testing
+ * the sequence.
+ */
+ snprintf(WALFilePath, sizeof(WALFilePath), "%s/%s",
+ archiveLocation, xlde->d_name);
+
+ if (dryrun)
+ {
/*
- * Use the original file name again now, including any
- * extension that might have been chopped off before testing
- * the sequence.
+ * Prints the name of the file to be removed and skips the
+ * actual removal. The regular printout is so that the
+ * user can pipe the output into some other program.
*/
- snprintf(WALFilePath, sizeof(WALFilePath), "%s/%s",
- archiveLocation, xlde->d_name);
-
- if (dryrun)
- {
- /*
- * Prints the name of the file to be removed and skips the
- * actual removal. The regular printout is so that the
- * user can pipe the output into some other program.
- */
- printf("%s\n", WALFilePath);
- pg_log_debug("file \"%s\" would be removed", WALFilePath);
- continue;
- }
-
- pg_log_debug("removing file \"%s\"", WALFilePath);
-
- rc = unlink(WALFilePath);
- if (rc != 0)
- pg_fatal("could not remove file \"%s\": %m",
- WALFilePath);
+ printf("%s\n", WALFilePath);
+ pg_log_debug("file \"%s\" would be removed", WALFilePath);
+ continue;
}
+
+ pg_log_debug("removing file \"%s\"", WALFilePath);
+
+ rc = unlink(WALFilePath);
+ if (rc != 0)
+ pg_fatal("could not remove file \"%s\": %m",
+ WALFilePath);
}
if (errno)
@@ -254,6 +267,7 @@ usage(void)
printf(_("\nOptions:\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"));
@@ -275,6 +289,7 @@ int
main(int argc, char **argv)
{
static struct option long_options[] = {
+ {"clean-backup-history", no_argument, NULL, 'b'},
{"debug", no_argument, NULL, 'd'},
{"dry-run", no_argument, NULL, 'n'},
{"strip-extension", required_argument, NULL, 'x'},
@@ -300,10 +315,13 @@ main(int argc, char **argv)
}
}
- while ((c = getopt_long(argc, argv, "dnx:", long_options, NULL)) != -1)
+ while ((c = getopt_long(argc, argv, "bdnx:", long_options, NULL)) != -1)
{
switch (c)
{
+ case 'b': /* Remove backup history files too */
+ cleanBackupHistory = 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 cc3386d146..ee2f714c2a 100644
--- a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
+++ b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
@@ -12,13 +12,18 @@ program_options_handling_ok('pg_archivecleanup');
my $tempdir = PostgreSQL::Test::Utils::tempdir;
-my @walfiles = (
+my @walfiles_with_gz = (
'00000001000000370000000C.gz', '00000001000000370000000D',
'00000001000000370000000E', '00000001000000370000000F.partial',);
+my @walfiles_with_backup = (
+ '00000001000000370000000D',
+ '00000001000000370000000D.00000028.backup',
+ '00000001000000370000000E', '00000001000000370000000F.partial',);
+
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_with_gz);
command_fails_like(
['pg_archivecleanup'],
@@ -59,14 +64,14 @@ command_fails_like(
my $stderr;
my $result =
IPC::Run::run [ 'pg_archivecleanup', '-d', '-n', $tempdir,
- $walfiles[2] ],
+ $walfiles_with_gz[2] ],
'2>', \$stderr;
ok($result, "pg_archivecleanup dry run: exit code 0");
like(
$stderr,
- qr/$walfiles[1].*would be removed/,
+ qr/$walfiles_with_gz[1].*would be removed/,
"pg_archivecleanup dry run: matches");
- foreach my $fn (@walfiles)
+ foreach my $fn (@walfiles_with_gz)
{
ok(-f "$tempdir/$fn", "$fn not removed");
}
@@ -76,32 +81,37 @@ sub run_check
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
- my ($suffix, $test_name) = @_;
+ my ($walfiles, $suffix, $test_name, @options) = @_;
- create_files();
+ create_files(\@$walfiles);
command_ok(
[
- 'pg_archivecleanup', '-x', '.gz', $tempdir,
- $walfiles[2] . $suffix
+ 'pg_archivecleanup', @options, $tempdir,
+ @$walfiles[2] . $suffix
],
"$test_name: runs");
- ok(!-f "$tempdir/$walfiles[0]",
+ ok(!-f "$tempdir/@$walfiles[0]",
"$test_name: first older WAL file was cleaned up");
- 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]",
"$test_name: restartfile was not cleaned up");
- ok(-f "$tempdir/$walfiles[3]",
+ ok(-f "$tempdir/@$walfiles[3]",
"$test_name: newer WAL 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');
+run_check(\@walfiles_with_gz, '',
+ 'pg_archivecleanup', '-x.gz');
+run_check(\@walfiles_with_gz, '.partial',
+ 'pg_archivecleanup with .partial file', '-x.gz');
+run_check(\@walfiles_with_gz, '.00000020.backup',
+ 'pg_archivecleanup with .backup file', '-x.gz');
+run_check(\@walfiles_with_backup, '',
+ 'pg_archivecleanup with --clean-backup-history', '-b');
done_testing();
--
2.39.2
