Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)

[Michael Paquier]

On Fri, Sep 23, 2022 at 09:12:22PM +0900, Michael Paquier wrote:
> I've read this one, and nothing is standing out at quick glance, so
> that looks rather reasonable to me.  I should be able to spend more
> time on that at the beginning of next week, and maybe apply it.

What I had at hand seemed fine on a second look, so applied after
tweaking a couple of comments.  One thing that I have been wondering
after-the-fact is whether it would be cleaner to return the contents
of the backup history file or backup_label as a char rather than a
StringInfo?  This simplifies a bit what the callers of
build_backup_content() need to do.
--
Michael

diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h
index cb15b8b80a..8ec3d88b0a 100644
--- a/src/include/access/xlogbackup.h
+++ b/src/include/access/xlogbackup.h
@@ -15,7 +15,6 @@
 #define XLOG_BACKUP_H
 
 #include "access/xlogdefs.h"
-#include "lib/stringinfo.h"
 #include "pgtime.h"
 
 /* Structure to hold backup state. */
@@ -36,7 +35,7 @@ typedef struct BackupState
 	pg_time_t	stoptime;		/* backup stop time */
 } BackupState;
 
-extern StringInfo build_backup_content(BackupState *state,
-									   bool ishistoryfile);
+extern char *build_backup_content(BackupState *state,
+								  bool ishistoryfile);
 
 #endif							/* XLOG_BACKUP_H */
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7606ee128a..1dd6df0fe1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8711,7 +8711,7 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
 	}
 	else
 	{
-		StringInfo	history_file;
+		char	   *history_file;
 
 		/*
 		 * Write the backup-end xlog record
@@ -8751,8 +8751,7 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
 
 		/* Build and save the contents of the backup history file */
 		history_file = build_backup_content(state, true);
-		fprintf(fp, "%s", history_file->data);
-		pfree(history_file->data);
+		fprintf(fp, "%s", history_file);
 		pfree(history_file);
 
 		if (fflush(fp) || ferror(fp) || FreeFile(fp))
diff --git a/src/backend/access/transam/xlogbackup.c b/src/backend/access/transam/xlogbackup.c
index c01c1f9010..90b5273b02 100644
--- a/src/backend/access/transam/xlogbackup.c
+++ b/src/backend/access/transam/xlogbackup.c
@@ -23,15 +23,16 @@
  * When ishistoryfile is true, it creates the contents for a backup history
  * file, otherwise it creates contents for a backup_label file.
  *
- * Returns the result generated as a palloc'd StringInfo.
+ * Returns the result generated as a palloc'd string.
  */
-StringInfo
+char *
 build_backup_content(BackupState *state, bool ishistoryfile)
 {
 	char		startstrbuf[128];
 	char		startxlogfile[MAXFNAMELEN]; /* backup start WAL file */
 	XLogSegNo	startsegno;
 	StringInfo	result = makeStringInfo();
+	char	   *data;
 
 	Assert(state != NULL);
 
@@ -76,5 +77,8 @@ build_backup_content(BackupState *state, bool ishistoryfile)
 		appendStringInfo(result, "STOP TIMELINE: %u\n", state->stoptli);
 	}
 
-	return result;
+	data = result->data;
+	pfree(result);
+
+	return data;
 }
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index f724b18733..a801a94fe8 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -130,7 +130,7 @@ pg_backup_stop(PG_FUNCTION_ARGS)
 	Datum		values[PG_BACKUP_STOP_V2_COLS] = {0};
 	bool		nulls[PG_BACKUP_STOP_V2_COLS] = {0};
 	bool		waitforarchive = PG_GETARG_BOOL(0);
-	StringInfo	backup_label;
+	char	   *backup_label;
 	SessionBackupState status = get_backup_status();
 
 	/* Initialize attributes information in the tuple descriptor */
@@ -153,7 +153,7 @@ pg_backup_stop(PG_FUNCTION_ARGS)
 	backup_label = build_backup_content(backup_state, false);
 
 	values[0] = LSNGetDatum(backup_state->stoppoint);
-	values[1] = CStringGetTextDatum(backup_label->data);
+	values[1] = CStringGetTextDatum(backup_label);
 	values[2] = CStringGetTextDatum(tablespace_map->data);
 
 	/* Deallocate backup-related variables */
@@ -162,7 +162,6 @@ pg_backup_stop(PG_FUNCTION_ARGS)
 	pfree(tablespace_map->data);
 	pfree(tablespace_map);
 	tablespace_map = NULL;
-	pfree(backup_label->data);
 	pfree(backup_label);
 
 	/* Returns the record as Datum */
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 495bbb506a..411cac9be3 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -317,15 +317,14 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 			{
 				struct stat statbuf;
 				bool		sendtblspclinks = true;
-				StringInfo	backup_label;
+				char	   *backup_label;
 
 				bbsink_begin_archive(sink, "base.tar");
 
 				/* In the main tar, include the backup_label first... */
 				backup_label = build_backup_content(backup_state, false);
 				sendFileWithContent(sink, BACKUP_LABEL_FILE,
-									backup_label->data, &manifest);
-				pfree(backup_label->data);
+									backup_label, &manifest);
 				pfree(backup_label);
 
 				/* Then the tablespace_map file, if required... */

signature.asc
Description: PGP signature