On Wed, Sep 21, 2022 at 05:10:39PM +0530, Bharath Rupireddy wrote:
>> deallocate_backup_variables() is the only part of xlogbackup.c that
>> includes references of the tablespace map_and backup_label
>> StringInfos.  I would be tempted to fully decouple that from
>> xlogbackup.c/h for the time being.
> 
> There's no problem with it IMO, after all, they are backup related
> variables. And that function reduces a bit of duplicate code.

Hmm.  I'd like to disagree with this statement :)

> Added that part before pg_backup_stop() now where it makes sense with
> the refactoring.

I have put my hands on 0001, and finished with the attached, that
includes many fixes and tweaks.  Some of the variable renames felt out
of place, while some comments were overly verbose for their purpose,
though for the last part we did not lose any information in the last
version proposed.

As I suspected, the deallocate routine felt unnecessary, as
xlogbackup.c/h have no idea what these are.  The remark is
particularly true for the StringInfo of the backup_label file: for
basebackup.c we need to build it when sending base.tar and in
xlogfuncs.c we need it only at the backup stop phase.  The code was
actually a bit wrong, because free-ing StringInfos requires to free
its ->data and then the main object (stringinfo.h explains that).  My
tweaks have shaved something like 10%~15% of the patch, while making
it IMO more readable.

A second issue I had was with the build function, and again it seemed
much cleaner to let the routine do the makeStringInfo() and return the
result.  This is not the most popular routine ever, but this reduces
the workload of the caller of build_backup_content().

So, opinions?
--
Michael
From 22216c4b6b75607d45e49620264b1af606396bd4 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 22 Sep 2022 16:41:55 +0900
Subject: [PATCH v11] Refactor backup related code

Refactor backup related code, advantages of doing so are following:

1) backup state is more structured now - all in a single structure,
callers can create backup_label contents whenever required, either
during the pg_backup_start or the pg_backup_stop or in between.
2) no parsing of backup_label file lines now, no error checking
for invalid parsing.
3) backup_label and history file contents have most of the things
in common, they can now be created within a single function.
4) makes backup related code extensible and readable.

This introduces new source files xlogbackup.c/.h for backup related
code and adds the new code in there. The xlog.c file has already grown
to ~9000 LOC (as of this time). Eventually, we would want to move
all the backup related code from xlog.c, xlogfuncs.c, elsewhere to
here.

Author: Bharath Rupireddy
Reviewed-by: Michael Paquier
Reviewed-by: Fujii Masao
Discussion: https://www.postgresql.org/message-id/CALj2ACVqNUEXkaMKyHHOdvScfN9E4LuCWsX_R-YRNfzQ727CdA%40mail.gmail.com
---
 src/include/access/xlog.h               |  10 +-
 src/include/access/xlogbackup.h         |  42 +++++
 src/backend/access/transam/Makefile     |   1 +
 src/backend/access/transam/xlog.c       | 224 ++++++++----------------
 src/backend/access/transam/xlogbackup.c |  81 +++++++++
 src/backend/access/transam/xlogfuncs.c  |  96 ++++++----
 src/backend/backup/basebackup.c         |  44 +++--
 7 files changed, 298 insertions(+), 200 deletions(-)
 create mode 100644 src/include/access/xlogbackup.h
 create mode 100644 src/backend/access/transam/xlogbackup.c

diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 3dbfa6b593..dce265098e 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -11,6 +11,7 @@
 #ifndef XLOG_H
 #define XLOG_H
 
+#include "access/xlogbackup.h"
 #include "access/xlogdefs.h"
 #include "access/xlogreader.h"
 #include "datatype/timestamp.h"
@@ -277,11 +278,10 @@ typedef enum SessionBackupState
 	SESSION_BACKUP_RUNNING,
 } SessionBackupState;
 
-extern XLogRecPtr do_pg_backup_start(const char *backupidstr, bool fast,
-									 TimeLineID *starttli_p, StringInfo labelfile,
-									 List **tablespaces, StringInfo tblspcmapfile);
-extern XLogRecPtr do_pg_backup_stop(char *labelfile, bool waitforarchive,
-									TimeLineID *stoptli_p);
+extern void do_pg_backup_start(const char *backupidstr, bool fast,
+							   List **tablespaces, BackupState *state,
+							   StringInfo tblspcmapfile);
+extern void do_pg_backup_stop(BackupState *state, bool waitforarchive);
 extern void do_pg_abort_backup(int code, Datum arg);
 extern void register_persistent_abort_backup_handler(void);
 extern SessionBackupState get_backup_status(void);
diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h
new file mode 100644
index 0000000000..ccdfefe117
--- /dev/null
+++ b/src/include/access/xlogbackup.h
@@ -0,0 +1,42 @@
+/*-------------------------------------------------------------------------
+ *
+ * xlogbackup.h
+ *		Definitions for internals of base backups.
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ *		src/include/access/xlogbackup.h
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef XLOG_BACKUP_H
+#define XLOG_BACKUP_H
+
+#include "access/xlogdefs.h"
+#include "lib/stringinfo.h"
+#include "pgtime.h"
+
+/* Structure to hold backup state. */
+typedef struct BackupState
+{
+	/* Fields saved at backup start */
+	/* Backup label name one extra byte for null-termination */
+	char            name[MAXPGPATH + 1];
+	XLogRecPtr      startpoint;	/* backup start WAL location */
+	TimeLineID      starttli;	/* backup start TLI */
+	XLogRecPtr      checkpointloc;	/* last checkpoint location */
+	pg_time_t       starttime;	/* backup start time */
+	bool            started_in_recovery;    /* backup started in recovery? */
+
+	/* Fields saved at the end of backup */
+	XLogRecPtr      stoppoint;	/* backup stop WAL location */
+	TimeLineID	    stoptli;	/* backup stop TLI */
+	pg_time_t	    stoptime;	/* backup stop time */
+} BackupState;
+
+extern StringInfo build_backup_content(BackupState *state,
+									   bool ishistoryfile);
+
+#endif							/* XLOG_BACKUP_H */
diff --git a/src/backend/access/transam/Makefile b/src/backend/access/transam/Makefile
index 3e5444a6f7..661c55a9db 100644
--- a/src/backend/access/transam/Makefile
+++ b/src/backend/access/transam/Makefile
@@ -29,6 +29,7 @@ OBJS = \
 	xact.o \
 	xlog.o \
 	xlogarchive.o \
+	xlogbackup.o \
 	xlogfuncs.o \
 	xloginsert.o \
 	xlogprefetcher.o \
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f32b2124e6..e061554363 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8242,24 +8242,23 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
 /*
  * do_pg_backup_start is the workhorse of the user-visible pg_backup_start()
  * function. It creates the necessary starting checkpoint and constructs the
- * backup label and tablespace map.
+ * backup state and tablespace map.
  *
- * Input parameters are "backupidstr" (the backup label string) and "fast"
- * (if true, we do the checkpoint in immediate mode to make it faster).
+ * Input parameters are "state" (the backup state), "fast" (if true, we do
+ * the checkpoint in immediate mode to make it faster), and "tablespaces"
+ * (if non-NULL, indicates a list of tablespaceinfo structs describing the
+ * cluster's tablespaces.).
  *
- * The backup label and tablespace map contents are appended to *labelfile and
- * *tblspcmapfile, and the caller is responsible for including them in the
- * backup archive as 'backup_label' and 'tablespace_map'.
- * tblspcmapfile is required mainly for tar format in windows as native windows
- * utilities are not able to create symlinks while extracting files from tar.
- * However for consistency and platform-independence, we do it the same way
- * everywhere.
+ * The tablespace map contents are appended to passed-in parameter
+ * tablespace_map and the caller is responsible for including it in the backup
+ * archive as 'tablespace_map'. The tablespace_map file is required mainly for
+ * tar format in windows as native windows utilities are not able to create
+ * symlinks while extracting files from tar. However for consistency and
+ * platform-independence, we do it the same way everywhere.
  *
- * If "tablespaces" isn't NULL, it receives a list of tablespaceinfo structs
- * describing the cluster's tablespaces.
- *
- * Returns the minimum WAL location that must be present to restore from this
- * backup, and the corresponding timeline ID in *starttli_p.
+ * It fills in backup_state with the information required for the backup,
+ * such as the minimum WAL location that must be present to restore from
+ * this backup (starttli) and the corresponding timeline ID (starttli).
  *
  * Every successfully started backup must be stopped by calling
  * do_pg_backup_stop() or do_pg_abort_backup(). There can be many
@@ -8268,20 +8267,13 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
  * It is the responsibility of the caller of this function to verify the
  * permissions of the calling user!
  */
-XLogRecPtr
-do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p,
-				   StringInfo labelfile, List **tablespaces,
-				   StringInfo tblspcmapfile)
+void
+do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
+				   BackupState *state, StringInfo tblspcmapfile)
 {
 	bool		backup_started_in_recovery = false;
-	XLogRecPtr	checkpointloc;
-	XLogRecPtr	startpoint;
-	TimeLineID	starttli;
-	pg_time_t	stamp_time;
-	char		strfbuf[128];
-	char		xlogfilename[MAXFNAMELEN];
-	XLogSegNo	_logSegNo;
 
+	Assert(state != NULL);
 	backup_started_in_recovery = RecoveryInProgress();
 
 	/*
@@ -8300,6 +8292,8 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 				 errmsg("backup label too long (max %d bytes)",
 						MAXPGPATH)));
 
+	memcpy(state->name, backupidstr, strlen(backupidstr));
+
 	/*
 	 * Mark backup active in shared memory.  We must do full-page WAL writes
 	 * during an on-line backup even if not doing so at other times, because
@@ -8391,9 +8385,9 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 			 * pointer.
 			 */
 			LWLockAcquire(ControlFileLock, LW_SHARED);
-			checkpointloc = ControlFile->checkPoint;
-			startpoint = ControlFile->checkPointCopy.redo;
-			starttli = ControlFile->checkPointCopy.ThisTimeLineID;
+			state->checkpointloc = ControlFile->checkPoint;
+			state->startpoint = ControlFile->checkPointCopy.redo;
+			state->starttli = ControlFile->checkPointCopy.ThisTimeLineID;
 			checkpointfpw = ControlFile->checkPointCopy.fullPageWrites;
 			LWLockRelease(ControlFileLock);
 
@@ -8410,7 +8404,7 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 				recptr = XLogCtl->lastFpwDisableRecPtr;
 				SpinLockRelease(&XLogCtl->info_lck);
 
-				if (!checkpointfpw || startpoint <= recptr)
+				if (!checkpointfpw || state->startpoint <= recptr)
 					ereport(ERROR,
 							(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 							 errmsg("WAL generated with full_page_writes=off was replayed "
@@ -8442,17 +8436,14 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 			 * either because only few buffers have been dirtied yet.
 			 */
 			WALInsertLockAcquireExclusive();
-			if (XLogCtl->Insert.lastBackupStart < startpoint)
+			if (XLogCtl->Insert.lastBackupStart < state->startpoint)
 			{
-				XLogCtl->Insert.lastBackupStart = startpoint;
+				XLogCtl->Insert.lastBackupStart = state->startpoint;
 				gotUniqueStartpoint = true;
 			}
 			WALInsertLockRelease();
 		} while (!gotUniqueStartpoint);
 
-		XLByteToSeg(startpoint, _logSegNo, wal_segment_size);
-		XLogFileName(xlogfilename, starttli, _logSegNo, wal_segment_size);
-
 		/*
 		 * Construct tablespace_map file.
 		 */
@@ -8538,39 +8529,16 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 		}
 		FreeDir(tblspcdir);
 
-		/*
-		 * Construct backup label file.
-		 */
-
-		/* Use the log timezone here, not the session timezone */
-		stamp_time = (pg_time_t) time(NULL);
-		pg_strftime(strfbuf, sizeof(strfbuf),
-					"%Y-%m-%d %H:%M:%S %Z",
-					pg_localtime(&stamp_time, log_timezone));
-		appendStringInfo(labelfile, "START WAL LOCATION: %X/%X (file %s)\n",
-						 LSN_FORMAT_ARGS(startpoint), xlogfilename);
-		appendStringInfo(labelfile, "CHECKPOINT LOCATION: %X/%X\n",
-						 LSN_FORMAT_ARGS(checkpointloc));
-		appendStringInfo(labelfile, "BACKUP METHOD: streamed\n");
-		appendStringInfo(labelfile, "BACKUP FROM: %s\n",
-						 backup_started_in_recovery ? "standby" : "primary");
-		appendStringInfo(labelfile, "START TIME: %s\n", strfbuf);
-		appendStringInfo(labelfile, "LABEL: %s\n", backupidstr);
-		appendStringInfo(labelfile, "START TIMELINE: %u\n", starttli);
+		state->starttime = (pg_time_t) time(NULL);
 	}
 	PG_END_ENSURE_ERROR_CLEANUP(pg_backup_start_callback, (Datum) 0);
 
+	state->started_in_recovery = backup_started_in_recovery;
+
 	/*
 	 * Mark that the start phase has correctly finished for the backup.
 	 */
 	sessionBackupState = SESSION_BACKUP_RUNNING;
-
-	/*
-	 * We're done.  As a convenience, return the starting WAL location.
-	 */
-	if (starttli_p)
-		*starttli_p = starttli;
-	return startpoint;
 }
 
 /* Error cleanup callback for pg_backup_start */
@@ -8602,48 +8570,38 @@ get_backup_status(void)
 /*
  * do_pg_backup_stop
  *
- * Utility function called at the end of an online backup. It cleans up the
- * backup state and can optionally wait for WAL segments to be archived.
+ * Utility function called at the end of an online backup.  It creates history
+ * file (if required), resets sessionBackupState and so on.  It can optionally
+ * wait for WAL segments to be archived.
  *
- * Returns the last WAL location that must be present to restore from this
- * backup, and the corresponding timeline ID in *stoptli_p.
+ * backup_state is filled with the information necessary to restore from this
+ * backup with its stop LSN (stoppoint), its timeline ID (stoptli), etc.
  *
  * It is the responsibility of the caller of this function to verify the
  * permissions of the calling user!
  */
-XLogRecPtr
-do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
+void
+do_pg_backup_stop(BackupState *state, bool waitforarchive)
 {
-	bool		backup_started_in_recovery = false;
-	XLogRecPtr	startpoint;
-	XLogRecPtr	stoppoint;
-	TimeLineID	stoptli;
-	pg_time_t	stamp_time;
-	char		strfbuf[128];
+	bool		backup_stopped_in_recovery = false;
 	char		histfilepath[MAXPGPATH];
-	char		startxlogfilename[MAXFNAMELEN];
-	char		stopxlogfilename[MAXFNAMELEN];
 	char		lastxlogfilename[MAXFNAMELEN];
 	char		histfilename[MAXFNAMELEN];
-	char		backupfrom[20];
 	XLogSegNo	_logSegNo;
 	FILE	   *fp;
-	char		ch;
 	int			seconds_before_warning;
 	int			waits = 0;
 	bool		reported_waiting = false;
-	char	   *remaining;
-	char	   *ptr;
-	uint32		hi,
-				lo;
 
-	backup_started_in_recovery = RecoveryInProgress();
+	Assert(state != NULL);
+
+	backup_stopped_in_recovery = RecoveryInProgress();
 
 	/*
 	 * During recovery, we don't need to check WAL level. Because, if WAL
 	 * level is not sufficient, it's impossible to get here during recovery.
 	 */
-	if (!backup_started_in_recovery && !XLogIsNeeded())
+	if (!backup_stopped_in_recovery && !XLogIsNeeded())
 		ereport(ERROR,
 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 				 errmsg("WAL level not sufficient for making an online backup"),
@@ -8684,29 +8642,11 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 	WALInsertLockRelease();
 
 	/*
-	 * Read and parse the START WAL LOCATION line (this code is pretty crude,
-	 * but we are not expecting any variability in the file format).
+	 * If we are taking an online backup from the standby, we confirm that the
+	 * standby has not been promoted during the backup.
 	 */
-	if (sscanf(labelfile, "START WAL LOCATION: %X/%X (file %24s)%c",
-			   &hi, &lo, startxlogfilename,
-			   &ch) != 4 || ch != '\n')
-		ereport(ERROR,
-				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-				 errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE)));
-	startpoint = ((uint64) hi) << 32 | lo;
-	remaining = strchr(labelfile, '\n') + 1;	/* %n is not portable enough */
-
-	/*
-	 * Parse the BACKUP FROM line. If we are taking an online backup from the
-	 * standby, we confirm that the standby has not been promoted during the
-	 * backup.
-	 */
-	ptr = strstr(remaining, "BACKUP FROM:");
-	if (!ptr || sscanf(ptr, "BACKUP FROM: %19s\n", backupfrom) != 1)
-		ereport(ERROR,
-				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-				 errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE)));
-	if (strcmp(backupfrom, "standby") == 0 && !backup_started_in_recovery)
+	if (state->started_in_recovery == true &&
+		backup_stopped_in_recovery == false)
 		ereport(ERROR,
 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 				 errmsg("the standby was promoted during online backup"),
@@ -8742,7 +8682,7 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 	 * an archiver is not invoked. So it doesn't seem worthwhile to write a
 	 * backup history file during recovery.
 	 */
-	if (backup_started_in_recovery)
+	if (backup_stopped_in_recovery)
 	{
 		XLogRecPtr	recptr;
 
@@ -8754,7 +8694,7 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 		recptr = XLogCtl->lastFpwDisableRecPtr;
 		SpinLockRelease(&XLogCtl->info_lck);
 
-		if (startpoint <= recptr)
+		if (state->startpoint <= recptr)
 			ereport(ERROR,
 					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 					 errmsg("WAL generated with full_page_writes=off was replayed "
@@ -8766,24 +8706,27 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 
 
 		LWLockAcquire(ControlFileLock, LW_SHARED);
-		stoppoint = ControlFile->minRecoveryPoint;
-		stoptli = ControlFile->minRecoveryPointTLI;
+		state->stoppoint = ControlFile->minRecoveryPoint;
+		state->stoptli = ControlFile->minRecoveryPointTLI;
 		LWLockRelease(ControlFileLock);
 	}
 	else
 	{
+		StringInfo history_file;
+
 		/*
 		 * Write the backup-end xlog record
 		 */
 		XLogBeginInsert();
-		XLogRegisterData((char *) (&startpoint), sizeof(startpoint));
-		stoppoint = XLogInsert(RM_XLOG_ID, XLOG_BACKUP_END);
+		XLogRegisterData((char *) (&state->startpoint),
+						 sizeof(state->startpoint));
+		state->stoppoint = XLogInsert(RM_XLOG_ID, XLOG_BACKUP_END);
 
 		/*
 		 * Given that we're not in recovery, InsertTimeLineID is set and can't
 		 * change, so we can read it without a lock.
 		 */
-		stoptli = XLogCtl->InsertTimeLineID;
+		state->stoptli = XLogCtl->InsertTimeLineID;
 
 		/*
 		 * Force a switch to a new xlog segment file, so that the backup is
@@ -8791,39 +8734,28 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 		 */
 		RequestXLogSwitch(false);
 
-		XLByteToPrevSeg(stoppoint, _logSegNo, wal_segment_size);
-		XLogFileName(stopxlogfilename, stoptli, _logSegNo, wal_segment_size);
-
-		/* Use the log timezone here, not the session timezone */
-		stamp_time = (pg_time_t) time(NULL);
-		pg_strftime(strfbuf, sizeof(strfbuf),
-					"%Y-%m-%d %H:%M:%S %Z",
-					pg_localtime(&stamp_time, log_timezone));
+		XLByteToPrevSeg(state->stoppoint, _logSegNo, wal_segment_size);
+		state->stoptime = (pg_time_t) time(NULL);
 
 		/*
-		 * Write the backup history file
+		 * Write the backup history file. Add backup_label file contents too it.
 		 */
-		XLByteToSeg(startpoint, _logSegNo, wal_segment_size);
-		BackupHistoryFilePath(histfilepath, stoptli, _logSegNo,
-							  startpoint, wal_segment_size);
+		XLByteToSeg(state->startpoint, _logSegNo, wal_segment_size);
+		BackupHistoryFilePath(histfilepath, state->stoptli, _logSegNo,
+							  state->startpoint, wal_segment_size);
 		fp = AllocateFile(histfilepath, "w");
 		if (!fp)
 			ereport(ERROR,
 					(errcode_for_file_access(),
 					 errmsg("could not create file \"%s\": %m",
 							histfilepath)));
-		fprintf(fp, "START WAL LOCATION: %X/%X (file %s)\n",
-				LSN_FORMAT_ARGS(startpoint), startxlogfilename);
-		fprintf(fp, "STOP WAL LOCATION: %X/%X (file %s)\n",
-				LSN_FORMAT_ARGS(stoppoint), stopxlogfilename);
 
-		/*
-		 * Transfer remaining lines including label and start timeline to
-		 * history file.
-		 */
-		fprintf(fp, "%s", remaining);
-		fprintf(fp, "STOP TIME: %s\n", strfbuf);
-		fprintf(fp, "STOP TIMELINE: %u\n", stoptli);
+		/* 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);
+		pfree(history_file);
+
 		if (fflush(fp) || ferror(fp) || FreeFile(fp))
 			ereport(ERROR,
 					(errcode_for_file_access(),
@@ -8861,15 +8793,16 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 	 */
 
 	if (waitforarchive &&
-		((!backup_started_in_recovery && XLogArchivingActive()) ||
-		 (backup_started_in_recovery && XLogArchivingAlways())))
+		((!backup_stopped_in_recovery && XLogArchivingActive()) ||
+		 (backup_stopped_in_recovery && XLogArchivingAlways())))
 	{
-		XLByteToPrevSeg(stoppoint, _logSegNo, wal_segment_size);
-		XLogFileName(lastxlogfilename, stoptli, _logSegNo, wal_segment_size);
+		XLByteToPrevSeg(state->stoppoint, _logSegNo, wal_segment_size);
+		XLogFileName(lastxlogfilename, state->stoptli, _logSegNo,
+					 wal_segment_size);
 
-		XLByteToSeg(startpoint, _logSegNo, wal_segment_size);
-		BackupHistoryFileName(histfilename, stoptli, _logSegNo,
-							  startpoint, wal_segment_size);
+		XLByteToSeg(state->startpoint, _logSegNo, wal_segment_size);
+		BackupHistoryFileName(histfilename, state->stoptli, _logSegNo,
+							  state->startpoint, wal_segment_size);
 
 		seconds_before_warning = 60;
 		waits = 0;
@@ -8910,13 +8843,6 @@ do_pg_backup_stop(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 	else if (waitforarchive)
 		ereport(NOTICE,
 				(errmsg("WAL archiving is not enabled; you must ensure that all required WAL segments are copied through other means to complete the backup")));
-
-	/*
-	 * We're done.  As a convenience, return the ending WAL location.
-	 */
-	if (stoptli_p)
-		*stoptli_p = stoptli;
-	return stoppoint;
 }
 
 
diff --git a/src/backend/access/transam/xlogbackup.c b/src/backend/access/transam/xlogbackup.c
new file mode 100644
index 0000000000..5b3a5cce73
--- /dev/null
+++ b/src/backend/access/transam/xlogbackup.c
@@ -0,0 +1,81 @@
+/*-------------------------------------------------------------------------
+ *
+ * xlogbackup.c
+ *		Internal routines for base backups.
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ *      src/backend/access/transam/xlogbackup.c
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "access/xlog.h"
+#include "access/xlog_internal.h"
+#include "access/xlogbackup.h"
+#include "utils/memutils.h"
+
+/*
+ * Build contents for backup_label or backup history file.
+ *
+ * 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.
+ */
+StringInfo
+build_backup_content(BackupState *state, bool ishistoryfile)
+{
+	char    startstrbuf[128];
+	char	startxlogfile[MAXFNAMELEN]; /* backup start WAL file */
+	XLogSegNo	startsegno;
+	StringInfo	result = makeStringInfo();
+
+	Assert(state != NULL);
+
+	/* Use the log timezone here, not the session timezone */
+	pg_strftime(startstrbuf, sizeof(startstrbuf), "%Y-%m-%d %H:%M:%S %Z",
+				pg_localtime(&state->starttime, log_timezone));
+
+	XLByteToSeg(state->startpoint, startsegno, wal_segment_size);
+	XLogFileName(startxlogfile, state->starttli, startsegno, wal_segment_size);
+	appendStringInfo(result, "START WAL LOCATION: %X/%X (file %s)\n",
+					 LSN_FORMAT_ARGS(state->startpoint), startxlogfile);
+
+	if (ishistoryfile)
+	{
+		char	stopxlogfile[MAXFNAMELEN];	/* backup stop WAL file */
+		XLogSegNo	stopsegno;
+
+		XLByteToSeg(state->startpoint, stopsegno, wal_segment_size);
+		XLogFileName(stopxlogfile, state->starttli, stopsegno, wal_segment_size);
+		appendStringInfo(result, "STOP WAL LOCATION: %X/%X (file %s)\n",
+						 LSN_FORMAT_ARGS(state->startpoint), stopxlogfile);
+	}
+
+	appendStringInfo(result, "CHECKPOINT LOCATION: %X/%X\n",
+					 LSN_FORMAT_ARGS(state->checkpointloc));
+	appendStringInfo(result, "BACKUP METHOD: streamed\n");
+	appendStringInfo(result, "BACKUP FROM: %s\n",
+					 state->started_in_recovery ? "standby" : "primary");
+	appendStringInfo(result, "START TIME: %s\n", startstrbuf);
+	appendStringInfo(result, "LABEL: %s\n", state->name);
+	appendStringInfo(result, "START TIMELINE: %u\n", state->starttli);
+
+	if (ishistoryfile)
+	{
+		char    stopstrfbuf[128];
+
+		/* Use the log timezone here, not the session timezone */
+		pg_strftime(stopstrfbuf, sizeof(stopstrfbuf), "%Y-%m-%d %H:%M:%S %Z",
+					pg_localtime(&state->stoptime, log_timezone));
+
+		appendStringInfo(result, "STOP TIME: %s\n", stopstrfbuf);
+		appendStringInfo(result, "STOP TIMELINE: %u\n", state->stoptli);
+	}
+
+	return result;
+}
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 27aeb6e281..7925ceeaad 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -20,6 +20,7 @@
 
 #include "access/htup_details.h"
 #include "access/xlog_internal.h"
+#include "access/xlogbackup.h"
 #include "access/xlogrecovery.h"
 #include "access/xlogutils.h"
 #include "catalog/pg_type.h"
@@ -39,19 +40,16 @@
 #include "utils/tuplestore.h"
 
 /*
- * Store label file and tablespace map during backups.
+ * Backup-related variables.
  */
-static StringInfo label_file;
-static StringInfo tblspc_map_file;
+static BackupState *backup_state = NULL;
+static StringInfo tablespace_map = NULL;
 
 /*
  * pg_backup_start: set up for taking an on-line backup dump
  *
- * Essentially what this does is to create a backup label file in $PGDATA,
- * where it will be archived as part of the backup dump.  The label file
- * contains the user-supplied label string (typically this would be used
- * to tell where the backup dump will be stored) and the starting time and
- * starting WAL location for the dump.
+ * Essentially what this does is to create the contents required for the
+ * backup_label file and the tablespace map.
  *
  * Permission checking for this function is managed through the normal
  * GRANT system.
@@ -62,7 +60,6 @@ pg_backup_start(PG_FUNCTION_ARGS)
 	text	   *backupid = PG_GETARG_TEXT_PP(0);
 	bool		fast = PG_GETARG_BOOL(1);
 	char	   *backupidstr;
-	XLogRecPtr	startpoint;
 	SessionBackupState status = get_backup_status();
 	MemoryContext oldcontext;
 
@@ -74,20 +71,40 @@ pg_backup_start(PG_FUNCTION_ARGS)
 				 errmsg("a backup is already in progress in this session")));
 
 	/*
-	 * Label file and tablespace map file need to be long-lived, since they
-	 * are read in pg_backup_stop.
+	 * backup_state and tablespace_map need to be long-lived as they are used
+	 * in pg_backup_stop().
 	 */
 	oldcontext = MemoryContextSwitchTo(TopMemoryContext);
-	label_file = makeStringInfo();
-	tblspc_map_file = makeStringInfo();
+
+	/* Allocate backup state or reset it, if it comes from a previous run */
+	if (backup_state == NULL)
+	{
+		backup_state = (BackupState *) palloc0(sizeof(BackupState));
+	}
+	else
+	{
+		MemSet(backup_state, 0, sizeof(BackupState));
+		MemSet(backup_state->name, '\0', sizeof(backup_state->name));
+	}
+
+	/*
+	 * tablespace_map may have been created in a previous backup, so take
+	 * this occasion to clean it.
+	 */
+	if (tablespace_map != NULL)
+	{
+		pfree(tablespace_map->data);
+		pfree(tablespace_map);
+		tablespace_map = NULL;
+	}
+
+	tablespace_map = makeStringInfo();
 	MemoryContextSwitchTo(oldcontext);
 
 	register_persistent_abort_backup_handler();
+	do_pg_backup_start(backupidstr, fast, NULL, backup_state, tablespace_map);
 
-	startpoint = do_pg_backup_start(backupidstr, fast, NULL, label_file,
-									NULL, tblspc_map_file);
-
-	PG_RETURN_LSN(startpoint);
+	PG_RETURN_LSN(backup_state->startpoint);
 }
 
 
@@ -98,6 +115,15 @@ pg_backup_start(PG_FUNCTION_ARGS)
  * allows the user to choose if they want to wait for the WAL to be archived
  * or if we should just return as soon as the WAL record is written.
  *
+ * This function stops an in-progress backup, creates backup_label contents and
+ * it returns the backup stop LSN, backup_label and tablespace_map contents.
+ *
+ * The backup_label contains the user-supplied label string (typically this
+ * would be used to tell where the backup dump will be stored), the starting
+ * time, starting WAL location for the dump and so on.  It is the caller's
+ * responsibility to write the backup_label and tablespace_map files in the
+ * data folder that will be restored from this backup.
+ *
  * Permission checking for this function is managed through the normal
  * GRANT system.
  */
@@ -108,9 +134,8 @@ pg_backup_stop(PG_FUNCTION_ARGS)
 	TupleDesc	tupdesc;
 	Datum		values[PG_BACKUP_STOP_V2_COLS] = {0};
 	bool		nulls[PG_BACKUP_STOP_V2_COLS] = {0};
-
 	bool		waitforarchive = PG_GETARG_BOOL(0);
-	XLogRecPtr	stoppoint;
+	StringInfo	backup_label;
 	SessionBackupState status = get_backup_status();
 
 	/* Initialize attributes information in the tuple descriptor */
@@ -123,23 +148,28 @@ pg_backup_stop(PG_FUNCTION_ARGS)
 				 errmsg("backup is not in progress"),
 				 errhint("Did you call pg_backup_start()?")));
 
-	/*
-	 * Stop the backup. Return a copy of the backup label and tablespace map
-	 * so they can be written to disk by the caller.
-	 */
-	stoppoint = do_pg_backup_stop(label_file->data, waitforarchive, NULL);
+	Assert(backup_state != NULL);
+	Assert(tablespace_map != NULL);
 
-	values[0] = LSNGetDatum(stoppoint);
-	values[1] = CStringGetTextDatum(label_file->data);
-	values[2] = CStringGetTextDatum(tblspc_map_file->data);
+	/* Stop the backup */
+	do_pg_backup_stop(backup_state, waitforarchive);
+
+	/* Build the contents of backup_label */
+	backup_label = build_backup_content(backup_state, false);
+
+	values[0] = LSNGetDatum(backup_state->stoppoint);
+	values[1] = CStringGetTextDatum(backup_label->data);
+	values[2] = CStringGetTextDatum(tablespace_map->data);
 
 	/* Free structures allocated in TopMemoryContext */
-	pfree(label_file->data);
-	pfree(label_file);
-	label_file = NULL;
-	pfree(tblspc_map_file->data);
-	pfree(tblspc_map_file);
-	tblspc_map_file = NULL;
+	pfree(backup_state);
+	backup_state = NULL;
+	pfree(tablespace_map->data);
+	pfree(tablespace_map);
+	tablespace_map = NULL;
+	pfree(backup_label->data);
+	pfree(backup_label);
+	backup_label = NULL;
 
 	/* Returns the record as Datum */
 	PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)));
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index dd103a8689..703ee08aef 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -17,6 +17,7 @@
 #include <time.h>
 
 #include "access/xlog_internal.h"
+#include "access/xlogbackup.h"
 #include "backup/backup_manifest.h"
 #include "backup/basebackup.h"
 #include "backup/basebackup_sink.h"
@@ -231,9 +232,9 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 	bbsink_state state;
 	XLogRecPtr	endptr;
 	TimeLineID	endtli;
-	StringInfo	labelfile;
-	StringInfo	tblspc_map_file;
 	backup_manifest_info manifest;
+	BackupState *backup_state;
+	StringInfo	tablespace_map;
 
 	/* Initial backup state, insofar as we know it now. */
 	state.tablespaces = NIL;
@@ -248,18 +249,21 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 
 	backup_started_in_recovery = RecoveryInProgress();
 
-	labelfile = makeStringInfo();
-	tblspc_map_file = makeStringInfo();
 	InitializeBackupManifest(&manifest, opt->manifest,
 							 opt->manifest_checksum_type);
 
 	total_checksum_failures = 0;
 
+	/* Allocate backup related varilables. */
+	backup_state = (BackupState *) palloc0(sizeof(BackupState));
+	tablespace_map = makeStringInfo();
+
 	basebackup_progress_wait_checkpoint();
-	state.startptr = do_pg_backup_start(opt->label, opt->fastcheckpoint,
-										&state.starttli,
-										labelfile, &state.tablespaces,
-										tblspc_map_file);
+	do_pg_backup_start(opt->label, opt->fastcheckpoint, &state.tablespaces,
+					   backup_state, tablespace_map);
+
+	state.startptr = backup_state->startpoint;
+	state.starttli = backup_state->starttli;
 
 	/*
 	 * Once do_pg_backup_start has been called, ensure that any failure causes
@@ -313,18 +317,22 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 			{
 				struct stat statbuf;
 				bool		sendtblspclinks = true;
+				StringInfo	backup_label;
 
 				bbsink_begin_archive(sink, "base.tar");
 
 				/* In the main tar, include the backup_label first... */
-				sendFileWithContent(sink, BACKUP_LABEL_FILE, labelfile->data,
-									&manifest);
+				backup_label = build_backup_content(backup_state, false);
+				sendFileWithContent(sink, BACKUP_LABEL_FILE,
+									backup_label->data, &manifest);
+				pfree(backup_label->data);
+				pfree(backup_label);
 
 				/* Then the tablespace_map file, if required... */
 				if (opt->sendtblspcmapfile)
 				{
-					sendFileWithContent(sink, TABLESPACE_MAP, tblspc_map_file->data,
-										&manifest);
+					sendFileWithContent(sink, TABLESPACE_MAP,
+										tablespace_map->data, &manifest);
 					sendtblspclinks = false;
 				}
 
@@ -374,7 +382,17 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 		}
 
 		basebackup_progress_wait_wal_archive(&state);
-		endptr = do_pg_backup_stop(labelfile->data, !opt->nowait, &endtli);
+		do_pg_backup_stop(backup_state, !opt->nowait);
+
+		endptr = backup_state->stoppoint;
+		endtli = backup_state->stoptli;
+
+		/* Deallocate backup related variables. */
+		pfree(tablespace_map->data);
+		pfree(tablespace_map);
+		tablespace_map = NULL;
+		pfree(backup_state);
+		backup_state = NULL;
 	}
 	PG_END_ENSURE_ERROR_CLEANUP(do_pg_abort_backup, BoolGetDatum(false));
 
-- 
2.37.2

Attachment: signature.asc
Description: PGP signature

Reply via email to