small pg_combinebackup improvements

From 46d2ab10897626fd3d22282713e67e38945f2d60 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Wed, 30 Oct 2024 15:02:38 -0400
Subject: [PATCH v1 1/2] pg_combinebackup: When reconstructing, avoid double
 slash in filename.

This function is always called with a relative_path that ends in a
slash, so there's no need to insert a second one. So, don't. Instead,
add an assertion to verify that nothing gets broken in the future, and
adjust the comments.
---
 src/bin/pg_combinebackup/reconstruct.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/bin/pg_combinebackup/reconstruct.c b/src/bin/pg_combinebackup/reconstruct.c
index db3f968d271..ae8a5125263 100644
--- a/src/bin/pg_combinebackup/reconstruct.c
+++ b/src/bin/pg_combinebackup/reconstruct.c
@@ -77,8 +77,9 @@ static void read_block(rfile *s, off_t off, uint8 *buffer);
  *
  * relative_path should be the path to the directory containing this file,
  * relative to the root of the backup (NOT relative to the root of the
- * tablespace). bare_file_name should be the name of the file within that
- * directory, without "INCREMENTAL.".
+ * tablespace). It must always end with a trailing slash. bare_file_name
+ * should be the name of the file within that directory, without
+ * "INCREMENTAL.".
  *
  * n_prior_backups is the number of prior backups, and prior_backup_dirs is
  * an array of pathnames where those backups can be found.
@@ -111,6 +112,10 @@ reconstruct_from_incremental_file(char *input_filename,
 	rfile	   *copy_source = NULL;
 	pg_checksum_context checksum_ctx;
 
+	/* Sanity check the relative_path. */
+	Assert(relative_path[0] != '\0');
+	Assert(relative_path[strlen(relative_path) - 1] == '/');
+
 	/*
 	 * Every block must come either from the latest version of the file or
 	 * from one of the prior backups.
@@ -174,11 +179,11 @@ reconstruct_from_incremental_file(char *input_filename,
 		 * Look for the full file in the previous backup. If not found, then
 		 * look for an incremental file instead.
 		 */
-		snprintf(source_filename, MAXPGPATH, "%s/%s/%s",
+		snprintf(source_filename, MAXPGPATH, "%s/%s%s",
 				 prior_backup_dirs[sidx], relative_path, bare_file_name);
 		if ((s = make_rfile(source_filename, true)) == NULL)
 		{
-			snprintf(source_filename, MAXPGPATH, "%s/%s/INCREMENTAL.%s",
+			snprintf(source_filename, MAXPGPATH, "%s/%sINCREMENTAL.%s",
 					 prior_backup_dirs[sidx], relative_path, bare_file_name);
 			s = make_incremental_rfile(source_filename);
 		}
-- 
2.39.3 (Apple Git-145)

From 44625b0d549ce43ef5925ae12951167df94b9868 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Wed, 30 Oct 2024 15:33:22 -0400
Subject: [PATCH v1 2/2] pg_combinebackup: Error if incremental file exists in
 full backup.

Suppose that you run a command like "pg_combinebackup b1 b2 -o output",
but both b1 and b2 contain an INCREMENTAL.$something file in a directory
that is expected to contain relation files. This is an error, but the
previous code would not detect the problem and instead write a garbage
full file named $something to the output directory. This commit adds
code to detect the error and a test case to verify the behavior.
---
 src/bin/pg_combinebackup/meson.build          |  1 +
 src/bin/pg_combinebackup/reconstruct.c        |  8 +++
 .../pg_combinebackup/t/009_no_full_file.pl    | 66 +++++++++++++++++++
 3 files changed, 75 insertions(+)
 create mode 100644 src/bin/pg_combinebackup/t/009_no_full_file.pl

diff --git a/src/bin/pg_combinebackup/meson.build b/src/bin/pg_combinebackup/meson.build
index d142608e949..550d3503269 100644
--- a/src/bin/pg_combinebackup/meson.build
+++ b/src/bin/pg_combinebackup/meson.build
@@ -36,6 +36,7 @@ tests += {
       't/006_db_file_copy.pl',
       't/007_wal_level_minimal.pl',
       't/008_promote.pl',
+      't/009_no_full_file.pl',
     ],
   }
 }
diff --git a/src/bin/pg_combinebackup/reconstruct.c b/src/bin/pg_combinebackup/reconstruct.c
index ae8a5125263..37ae38b6108 100644
--- a/src/bin/pg_combinebackup/reconstruct.c
+++ b/src/bin/pg_combinebackup/reconstruct.c
@@ -326,11 +326,19 @@ reconstruct_from_incremental_file(char *input_filename,
 	 * result, then forget about performing reconstruction and just copy that
 	 * file in its entirety.
 	 *
+	 * If we have only incremental files, and there's no full file at any
+	 * point in the backup chain, something has gone wrong. Emit an error.
+	 *
 	 * Otherwise, reconstruct.
 	 */
 	if (copy_source != NULL)
 		copy_file(copy_source->filename, output_filename,
 				  &checksum_ctx, copy_method, dry_run);
+	else if (sidx == 0 && source[0]->header_length != 0)
+	{
+		pg_fatal("full backup contains unexpected incremental file \"%s\"",
+				 source[0]->filename);
+	}
 	else
 	{
 		write_reconstructed_file(input_filename, output_filename,
diff --git a/src/bin/pg_combinebackup/t/009_no_full_file.pl b/src/bin/pg_combinebackup/t/009_no_full_file.pl
new file mode 100644
index 00000000000..8a23fa7a4cf
--- /dev/null
+++ b/src/bin/pg_combinebackup/t/009_no_full_file.pl
@@ -0,0 +1,66 @@
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+use strict;
+use warnings FATAL => 'all';
+use File::Copy;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Can be changed to test the other modes.
+my $mode = $ENV{PG_TEST_PG_COMBINEBACKUP_MODE} || '--copy';
+
+note "testing using mode $mode";
+
+# Set up a new database instance.
+my $primary = PostgreSQL::Test::Cluster->new('primary');
+$primary->init(has_archiving => 1, allows_streaming => 1);
+$primary->append_conf('postgresql.conf', 'summarize_wal = on');
+$primary->start;
+
+# Take a full backup.
+my $backup1path = $primary->backup_dir . '/backup1';
+$primary->command_ok(
+	[ 'pg_basebackup', '-D', $backup1path, '--no-sync', '-cfast' ],
+	"full backup");
+
+# Take an incremental backup.
+my $backup2path = $primary->backup_dir . '/backup2';
+$primary->command_ok(
+	[
+		'pg_basebackup', '-D', $backup2path, '--no-sync', '-cfast',
+		'--incremental', $backup1path . '/backup_manifest'
+	],
+	"incremental backup");
+
+# Find an incremental file in the incremental backup for which there is a full
+# file in the full backup. When we find one, replace the full file with an
+# incremental file.
+my @filelist = grep { /^INCREMENTAL\./ } slurp_dir("$backup2path/base/1");
+my $success = 0;
+for my $iname (@filelist)
+{
+	my $name = $iname;
+	$name =~ s/^INCREMENTAL.//;
+
+	if (-f "$backup1path/base/1/$name")
+	{
+		copy("$backup2path/base/1/$iname", "$backup1path/base/1/$iname")
+			|| die "copy $backup2path/base/1/$iname: $!";
+		unlink("$backup1path/base/1/$name")
+			|| die "unlink $backup1path/base/1/$name: $!";
+		$success = 1;
+		last;
+	}
+}
+
+# pg_combinebackup should fail.
+my $outpath = $primary->backup_dir . '/out';
+$primary->command_fails_like(
+	[
+		'pg_combinebackup', $backup1path, $backup2path, '-o', $outpath,
+	],
+	qr/full backup contains unexpected incremental file/,
+	"pg_combinebackup fails");
+
+done_testing();
-- 
2.39.3 (Apple Git-145)