Feedback-ID: ia2694551:Fastmail
Date: Fri, 17 Nov 2023 11:01:21 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>,
	Andres Freund <andres@anarazel.de>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: trying again to get incremental backup
Message-ID: <202311171001.p5hwidowrrkj@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="7uz7kmqfyslbjyl6"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+TgmobsXQV3n10=R5PGhwnMEjtGt7AFu9EuxCCf1MwXMGgRnQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202311171001.p5hwidowrrkj%40alvherre.pgsql>
Precedence: bulk


--7uz7kmqfyslbjyl6
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

I made a pass over pg_combinebackup for NLS.  I propose the attached
patch.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Right now the sectors on the hard disk run clockwise, but I heard a rumor that
you can squeeze 0.2% more throughput by running them counterclockwise.
It's worth the effort. Recommended."  (Gerry Pourwelle)

--7uz7kmqfyslbjyl6
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-do-NLS-for-pg_combinebackup.patch"

From a385542ff03514885fa4e84b0485e51cdcdd04bd Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 17 Nov 2023 10:51:36 +0100
Subject: [PATCH] do NLS for pg_combinebackup

---
 src/bin/pg_combinebackup/backup_label.c     | 34 +++++++++++----------
 src/bin/pg_combinebackup/nls.mk             | 11 +++++++
 src/bin/pg_combinebackup/pg_combinebackup.c | 24 ++++++++++-----
 src/bin/pg_combinebackup/reconstruct.c      |  9 ++++--
 4 files changed, 52 insertions(+), 26 deletions(-)
 create mode 100644 src/bin/pg_combinebackup/nls.mk

diff --git a/src/bin/pg_combinebackup/backup_label.c b/src/bin/pg_combinebackup/backup_label.c
index 2a62aa6fad..922e00854d 100644
--- a/src/bin/pg_combinebackup/backup_label.c
+++ b/src/bin/pg_combinebackup/backup_label.c
@@ -63,18 +63,18 @@ parse_backup_label(char *filename, StringInfo buf,
 		if (line_starts_with(s, e, "START WAL LOCATION: ", &s))
 		{
 			if (!parse_lsn(s, e, start_lsn, &c))
-				pg_fatal("%s: could not parse START WAL LOCATION",
-						 filename);
+				pg_fatal("%s: could not parse %s",
+						 filename, "START WAL LOCATION");
 			if (c >= e || *c != ' ')
-				pg_fatal("%s: improper terminator for START WAL LOCATION",
-						 filename);
+				pg_fatal("%s: improper terminator for %s",
+						 filename, "START WAL LOCATION");
 			found |= 1;
 		}
 		else if (line_starts_with(s, e, "START TIMELINE: ", &s))
 		{
 			if (!parse_tli(s, e, start_tli))
-				pg_fatal("%s: could not parse TLI for START TIMELINE",
-						 filename);
+				pg_fatal("%s: could not parse TLI for %s",
+						 filename, "START TIMELINE");
 			if (*start_tli == 0)
 				pg_fatal("%s: invalid TLI", filename);
 			found |= 2;
@@ -82,18 +82,18 @@ parse_backup_label(char *filename, StringInfo buf,
 		else if (line_starts_with(s, e, "INCREMENTAL FROM LSN: ", &s))
 		{
 			if (!parse_lsn(s, e, previous_lsn, &c))
-				pg_fatal("%s: could not parse INCREMENTAL FROM LSN",
-						 filename);
+				pg_fatal("%s: could not parse %s",
+						 filename, "INCREMENTAL FROM LSN");
 			if (c >= e || *c != '\n')
-				pg_fatal("%s: improper terminator for INCREMENTAL FROM LSN",
-						 filename);
+				pg_fatal("%s: improper terminator for %s",
+						 filename, "INCREMENTAL FROM LSN");
 			found |= 4;
 		}
 		else if (line_starts_with(s, e, "INCREMENTAL FROM TLI: ", &s))
 		{
 			if (!parse_tli(s, e, previous_tli))
-				pg_fatal("%s: could not parse INCREMENTAL FROM TLI",
-						 filename);
+				pg_fatal("%s: could not parse %s",
+						 filename, "INCREMENTAL FROM TLI");
 			if (*previous_tli == 0)
 				pg_fatal("%s: invalid TLI", filename);
 			found |= 8;
@@ -103,13 +103,15 @@ parse_backup_label(char *filename, StringInfo buf,
 	}
 
 	if ((found & 1) == 0)
-		pg_fatal("%s: could not find START WAL LOCATION", filename);
+		pg_fatal("%s: could not find %s", filename, "START WAL LOCATION");
 	if ((found & 2) == 0)
-		pg_fatal("%s: could not find START TIMELINE", filename);
+		pg_fatal("%s: could not find %s", filename, "START TIMELINE");
 	if ((found & 4) != 0 && (found & 8) == 0)
-		pg_fatal("%s: INCREMENTAL FROM LSN requires INCREMENTAL FROM TLI", filename);
+		pg_fatal("%s: %s requires %s", filename,
+				 "INCREMENTAL FROM LSN", "INCREMENTAL FROM TLI");
 	if ((found & 8) != 0 && (found & 4) == 0)
-		pg_fatal("%s: INCREMENTAL FROM TLI requires INCREMENTAL FROM LSN", filename);
+		pg_fatal("%s: %s requires %s", filename,
+				 "INCREMENTAL FROM TLI", "INCREMENTAL FROM LSN");
 }
 
 /*
diff --git a/src/bin/pg_combinebackup/nls.mk b/src/bin/pg_combinebackup/nls.mk
new file mode 100644
index 0000000000..c8e59d1d00
--- /dev/null
+++ b/src/bin/pg_combinebackup/nls.mk
@@ -0,0 +1,11 @@
+# src/bin/pg_combinebackup/nls.mk
+CATALOG_NAME     = pg_combinebackup
+GETTEXT_FILES    = $(FRONTEND_COMMON_GETTEXT_FILES) \
+		   backup_label.c \
+		   copy_file.c \
+		   load_manifest.c \
+		   pg_combinebackup.c \
+		   reconstruct.c \
+		   write_manifest.c
+GETTEXT_TRIGGERS = $(FRONTEND_COMMON_GETTEXT_TRIGGERS)
+GETTEXT_FLAGS    = $(FRONTEND_COMMON_GETTEXT_FLAGS)
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 7bf56e57ae..618b5dd7f6 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -521,29 +521,33 @@ check_control_files(int n_backups, char **backup_dirs)
 	{
 		ControlFileData *control_file;
 		bool		crc_ok;
+		char	   *controlpath;
 
-		pg_log_debug("reading \"%s/global/pg_control\"", backup_dirs[i]);
+		path = psprintf("%s/%s", backup_dirs[i], "global/pg_control");
+
+		pg_log_debug("reading \"%s\"", controlpath);
 		control_file = get_controlfile(backup_dirs[i], &crc_ok);
 
 		/* Control file contents not meaningful if CRC is bad. */
 		if (!crc_ok)
-			pg_fatal("%s/global/pg_control: crc is incorrect", backup_dirs[i]);
+			pg_fatal("%s: crc is incorrect", controlpath);
 
 		/* Can't interpret control file if not current version. */
 		if (control_file->pg_control_version != PG_CONTROL_VERSION)
-			pg_fatal("%s/global/pg_control: unexpected control file version",
-					 backup_dirs[i]);
+			pg_fatal("%s: unexpected control file version",
+					 controlpath);
 
 		/* System identifiers should all match. */
 		if (i == n_backups - 1)
 			system_identifier = control_file->system_identifier;
 		else if (system_identifier != control_file->system_identifier)
-			pg_fatal("%s/global/pg_control: expected system identifier %llu, but found %llu",
-					 backup_dirs[i], (unsigned long long) system_identifier,
+			pg_fatal("%s: expected system identifier %llu, but found %llu",
+					 controlpath, (unsigned long long) system_identifier,
 					 (unsigned long long) control_file->system_identifier);
 
 		/* Release memory. */
 		pfree(control_file);
+		pfree(controlpath);
 	}
 
 	/*
@@ -932,12 +936,16 @@ process_directory_recursively(Oid tsoid,
 											  manifest_path);
 				if (mfile == NULL)
 				{
+					char	   *fullpath = psprintf("%s/%s", input_directory,
+													backup_manifest);
+
 					/*
 					 * The directory is out of sync with the backup_manifest,
 					 * so emit a warning.
 					 */
-					pg_log_warning("\"%s/backup_manifest\" contains no entry for \"%s\"",
-								   input_directory, manifest_path);
+					pg_log_warning("\"%s\" contains no entry for \"%s\"",
+								   fullpath, manifest_path);
+					pfree(fullpath);
 				}
 				else if (mfile->checksum_type == checksum_type)
 				{
diff --git a/src/bin/pg_combinebackup/reconstruct.c b/src/bin/pg_combinebackup/reconstruct.c
index e7f0523fe9..19ab96904b 100644
--- a/src/bin/pg_combinebackup/reconstruct.c
+++ b/src/bin/pg_combinebackup/reconstruct.c
@@ -283,13 +283,18 @@ reconstruct_from_incremental_file(char *input_filename,
 									  manifest_path);
 		if (mfile == NULL)
 		{
+			char	   *path = psprintf("%s/backup_manifest",
+										prior_backup_dirs[copy_source_index]);
+
 			/*
 			 * The directory is out of sync with the backup_manifest, so emit
 			 * a warning.
 			 */
-			pg_log_warning("\"%s/backup_manifest\" contains no entry for \"%s\"",
-						   prior_backup_dirs[copy_source_index],
+			/*- translator: the first %s is a backup manifest file, the second is a file absent therein */
+			pg_log_warning("\"%s\" contains no entry for \"%s\"",
+						   path,
 						   manifest_path);
+			pfree(path);
 		}
 		else if (mfile->checksum_type == checksum_type)
 		{
-- 
2.39.2


--7uz7kmqfyslbjyl6--