On Tue, Sep 20, 2022 at 7:20 AM Michael Paquier <mich...@paquier.xyz> wrote:
>
> The main regression test suite should not include direct calls to
> pg_backup_start() or pg_backup_stop() as these depend on wal_level,
> and we spend a certain amount of resources in keeping the tests a
> maximum portable across such configurations, wal_level = minimal being
> one of them.  One portable example is in 001_stream_rep.pl.

Understood.

> > That's a good idea. I'm marking a flag if the label name overflows (>
> > MAXPGPATH), later using it in do_pg_backup_start() to error out. We
> > could've thrown error directly, but that changes the behaviour - right
> > now, first "
> > wal_level must be set to \"replica\" or \"logical\" at server start."
> > gets emitted and then label name overflow error - I don't want to do
> > that.
>
> -   if (strlen(backupidstr) > MAXPGPATH)
> +   if (state->name_overflowed == true)
>         ereport(ERROR,
>                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>                  errmsg("backup label too long (max %d bytes)",
> It does not strike me as a huge issue to force a truncation of such
> backup label names.  1024 is large enough for basically all users,
> in my opinion.  Or you could just truncate in the internal logic, but
> still complain at MAXPGPATH - 1 as the last byte would be for the zero
> termination.  In short, there is no need to complicate things with
> name_overflowed.

We currently allow MAXPGPATH bytes of label name excluding null
termination. I don't want to change this behaviour. In the attached v9
patch, I'm truncating the larger names to  MAXPGPATH + 1 bytes in
backup state (one extra byte for representing that the name has
overflown, and another extra byte for null termination).

> +   StringInfo      backup_label;   /* backup_label file contents */
> +   StringInfo      tablespace_map; /* tablespace_map file contents */
> +   StringInfo      history_file;   /* history file contents */
> IMV, repeating a point I already made once upthread, BackupState
> should hold none of these.  Let's just generate the contents of these
> files in the contexts where they are needed, making BackupState
> something to rely on to build them in the code paths where they are
> necessary.  This is going to make the reasoning around the memory
> contexts where each one of them is stored much easier and reduce the
> changes of bugs in the long-term.

I've separated out these variables from the backup state, please see
the attached v9 patch.

> > I've also taken help of the error callback mechanism to clean up the
> > allocated memory in case of a failure. For do_pg_abort_backup() cases,
> > I think we don't need to clean the memory because that gets called on
> > proc exit (before_shmem_exit()).
>
> Memory could still bloat while the process running the SQL functions
> is running depending on the error code path, anyway.

I didn't get your point. Can you please elaborate it? I think adding
error callbacks at the right places would free up the memory for us.
Please note that we already are causing memory leaks on HEAD today.

I addressed the above review comments. I also changed a wrong comment
[1] that lies before pg_backup_start() even after the removal of
exclusive backup.

I'm attaching v9 patch set herewith, 0001 - refactors the backup code
with backup state, 0002 - adds error callbacks to clean up the memory
allocated for backup variables. Please review them further.

[1]
 * 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.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From ab1e86ac7fb75a2d2219c7681ead40faf8c01446 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 20 Sep 2022 10:04:29 +0000
Subject: [PATCH v9] 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/backend/access/transam/Makefile     |   1 +
 src/backend/access/transam/xlog.c       | 247 +++++++++---------------
 src/backend/access/transam/xlogbackup.c | 151 +++++++++++++++
 src/backend/access/transam/xlogfuncs.c  |  90 +++++----
 src/backend/backup/basebackup.c         |  41 ++--
 src/include/access/xlog.h               |   9 +-
 src/include/access/xlogbackup.h         |  53 +++++
 7 files changed, 384 insertions(+), 208 deletions(-)
 create mode 100644 src/backend/access/transam/xlogbackup.c
 create mode 100644 src/include/access/xlogbackup.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 eb0430fe98..d25750e783 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8236,46 +8236,40 @@ 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.
- *
- * Input parameters are "backupidstr" (the backup label string) and "fast"
- * (if true, we do the checkpoint in immediate mode to make it faster).
- *
- * 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.
- *
- * 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.
+ * backup state.
+ *
+ * Input parameters are "state" (containing 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 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.
+ *
+ * It fills in backup state with information such as the minimum WAL location
+ * that must be present to restore from this backup (starttli), and the
+ * corresponding timeline ID (starttli), last checkpoint location
+ * (checkpointloc), start time of the backup (starttime), whether the backup started
+ * in recovery (started_in_recovery) and so on.
  *
  * Every successfully started backup must be stopped by calling
- * do_pg_backup_stop() or do_pg_abort_backup(). There can be many
- * backups active at the same time.
+ * do_pg_backup_stop() or do_pg_abort_backup(). There can be many backups
+ * active at the same time.
  *
  * 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(BackupState *state, bool fast, List **tablespaces,
+				   StringInfo tablespace_map)
 {
 	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();
 
 	/*
@@ -8288,7 +8282,7 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 				 errmsg("WAL level not sufficient for making an online backup"),
 				 errhint("wal_level must be set to \"replica\" or \"logical\" at server start.")));
 
-	if (strlen(backupidstr) > MAXPGPATH)
+	if (strlen(state->name) > MAXPGPATH)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 				 errmsg("backup label too long (max %d bytes)",
@@ -8385,9 +8379,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);
 
@@ -8404,7 +8398,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 "
@@ -8436,19 +8430,16 @@ 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.
+		 * Construct tablespace_map contents.
 		 */
 		datadirpathlen = strlen(DataDir);
 
@@ -8525,46 +8516,23 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 			if (tablespaces)
 				*tablespaces = lappend(*tablespaces, ti);
 
-			appendStringInfo(tblspcmapfile, "%s %s\n",
+			appendStringInfo(tablespace_map, "%s %s\n",
 							 ti->oid, escapedpath.data);
 
 			pfree(escapedpath.data);
 		}
 		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 */
@@ -8596,48 +8564,43 @@ 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.
+ * It fills in backup state with information such as the last WAL location that
+ * must be present to restore from this backup (stoppoint), and the
+ * corresponding timeline ID (stoptli), stop time of the backup (stoptime) and
+ * so on.
  *
  * It is the responsibility of the caller of this function to verify the
  * permissions of the calling user!
+ *
+ * It is the responsibility of the caller to create backup label contents by
+ * calling build_backup_content() with appropriate parameters.
  */
-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"),
@@ -8678,29 +8641,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 (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.
+	 * 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"),
@@ -8736,7 +8681,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;
 
@@ -8748,7 +8693,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 "
@@ -8760,24 +8705,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
@@ -8785,45 +8733,36 @@ 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);
+		/* Construct history file contents. */
+		history_file = makeStringInfo();
+		build_backup_content(state, true, history_file);
+
+		fprintf(fp, "%s", history_file->data);
+
 		if (fflush(fp) || ferror(fp) || FreeFile(fp))
 			ereport(ERROR,
 					(errcode_for_file_access(),
 					 errmsg("could not write file \"%s\": %m",
 							histfilepath)));
 
+		pfree(history_file->data);
+
 		/*
 		 * Clean out any no-longer-needed history files.  As a side effect,
 		 * this will post a .ready file for the newly created history file,
@@ -8855,15 +8794,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;
@@ -8904,13 +8844,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..38c37cc7db
--- /dev/null
+++ b/src/backend/access/transam/xlogbackup.c
@@ -0,0 +1,151 @@
+/*-------------------------------------------------------------------------
+ * xlogbackup.c
+ *
+ * This file contains backup related code.
+ *
+ * Copyright (c) 2022, PostgreSQL Global Development Group
+ *
+ * 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"
+
+static void fill_backup_label_name(BackupState *state, const char* name);
+
+/*
+ * Allocate backup state and fill in the passed-in label name.
+ *
+ * Note that the caller has to ensure right memory context is set for the
+ * backup state.
+ */
+BackupState *
+allocate_backup_state(const char *name)
+{
+	BackupState *state;
+
+	state = (BackupState *) palloc0(sizeof(BackupState));
+
+	fill_backup_label_name(state, name);
+
+	return state;
+}
+
+/*
+ * Reset backup state and its members and fill in the passed-in label name.
+ */
+void
+reset_backup_state(BackupState *state, const char *name)
+{
+	if (state == NULL)
+		return;	/* nothing to do */
+
+	MemSet(state, 0, sizeof(BackupState));
+	MemSet(state->name, '\0', sizeof(state->name));
+
+	fill_backup_label_name(state, name);
+}
+
+/*
+ * Thin helper function to fill label name (after truncating it, if necessary)
+ * in backup state.
+ */
+static void
+fill_backup_label_name(BackupState *state, const char* name)
+{
+	Size length;
+
+	length = strlen(name);
+
+	/*
+	 * Truncate the backup label name whose length is greater than MAXPGPATH
+	 * bytes as we will anyways error out later in do_pg_backup_start(). We
+	 * truncate it to be one byte more to figure out the name has actually
+	 * overflown.
+	 */
+	if (length > MAXPGPATH)
+		length = MAXPGPATH + 1;
+
+	memcpy(state->name, name, length);
+}
+
+/*
+ * Deallocate backup state, backup_label and tablespace_map variables.
+ */
+void
+deallocate_backup_variables(BackupState *state, StringInfo backup_label,
+							StringInfo tablespace_map)
+{
+	if (state != NULL)
+		pfree(state);
+
+	if (backup_label != NULL)
+		pfree(backup_label->data);
+
+	if (tablespace_map != NULL)
+		pfree(tablespace_map->data);
+}
+
+/*
+ * Construct backup_label file or history file contents
+ *
+ * When ishistoryfile is true, it creates contents for backup history file,
+ * otherwise, it creates contents for backup_label file.
+ */
+void
+build_backup_content(BackupState *state, bool ishistoryfile, StringInfo str)
+{
+	char    startstrbuf[128];
+	char	startxlogfile[MAXFNAMELEN]; /* backup start WAL file */
+	XLogSegNo	startsegno;
+
+	Assert(state != NULL);
+	Assert(str != 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(str, "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(str, "STOP WAL LOCATION: %X/%X (file %s)\n",
+						 LSN_FORMAT_ARGS(state->startpoint), stopxlogfile);
+	}
+
+	appendStringInfo(str, "CHECKPOINT LOCATION: %X/%X\n",
+					 LSN_FORMAT_ARGS(state->checkpointloc));
+	appendStringInfo(str, "BACKUP METHOD: streamed\n");
+	appendStringInfo(str, "BACKUP FROM: %s\n",
+					 state->started_in_recovery ? "standby" : "primary");
+	appendStringInfo(str, "START TIME: %s\n", startstrbuf);
+	appendStringInfo(str, "LABEL: %s\n", state->name);
+	appendStringInfo(str, "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(str, "STOP TIME: %s\n", stopstrfbuf);
+		appendStringInfo(str, "STOP TIMELINE: %u\n", state->stoptli);
+	}
+}
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 27aeb6e281..e078579e6f 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,15 @@
 #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
+ * pg_backup_start: start taking an on-line backup.
  *
- * 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.
+ * This function starts the backup and creates tablespace_map contents.
  *
  * Permission checking for this function is managed through the normal
  * GRANT system.
@@ -62,7 +59,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 +70,33 @@ 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(), hence allocate in TopMemoryContext.
 	 */
 	oldcontext = MemoryContextSwitchTo(TopMemoryContext);
-	label_file = makeStringInfo();
-	tblspc_map_file = makeStringInfo();
+
+	if (backup_state == NULL)
+		backup_state = allocate_backup_state(backupidstr);
+	else
+		reset_backup_state(backup_state, backupidstr);
+
+	/*
+	 * tablespace_map memory may have been enlarged previously, hence we free
+	 * it up and allocate again instead of resetting to save some memory.
+	 */
+	if (tablespace_map != NULL)
+	{
+		pfree(tablespace_map->data);
+		tablespace_map = NULL;
+	}
+
+	tablespace_map = makeStringInfo();
 	MemoryContextSwitchTo(oldcontext);
 
 	register_persistent_abort_backup_handler();
+	do_pg_backup_start(backup_state, fast, NULL, 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 +107,9 @@ 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.
+ *
  * Permission checking for this function is managed through the normal
  * GRANT system.
  */
@@ -108,9 +120,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 +134,36 @@ pg_backup_stop(PG_FUNCTION_ARGS)
 				 errmsg("backup is not in progress"),
 				 errhint("Did you call pg_backup_start()?")));
 
+	/*
+	 * Assert the fact that these backup variables are allocated in
+	 * pg_backup_start().
+	 */
+	Assert(backup_state != NULL);
+	Assert(tablespace_map != NULL);
+
 	/*
 	 * 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);
-
-	values[0] = LSNGetDatum(stoppoint);
-	values[1] = CStringGetTextDatum(label_file->data);
-	values[2] = CStringGetTextDatum(tblspc_map_file->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;
+	do_pg_backup_stop(backup_state, waitforarchive);
+
+	/*
+	 * Construct backup_label contents. Note that the backup_label doesn't have
+	 * to be in TopMemoryContext unlike backup_state and tablespace_map as it
+	 * is short-lived.
+	 */
+	backup_label = makeStringInfo();
+	build_backup_content(backup_state, false, backup_label);
+
+	values[0] = LSNGetDatum(backup_state->stoppoint);
+	values[1] = CStringGetTextDatum(backup_label->data);
+	values[2] = CStringGetTextDatum(tablespace_map->data);
+
+	/* Deallocate backup related variables. */
+	deallocate_backup_variables(backup_state, backup_label, tablespace_map);
+	backup_state = NULL;
+	backup_label = NULL;
+	tablespace_map = 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 1f1cff1a58..7ba4f340bc 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,10 @@ 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	backup_label;
+	StringInfo	tablespace_map;
 
 	/* Initial backup state, insofar as we know it now. */
 	state.tablespaces = NIL;
@@ -248,18 +250,25 @@ 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 = allocate_backup_state(opt->label);
+	backup_label = makeStringInfo();
+	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(backup_state, opt->fastcheckpoint,
+					   &state.tablespaces, tablespace_map);
+
+	/* Construct backup_label contents. */
+	build_backup_content(backup_state, false, backup_label);
+
+	state.startptr = backup_state->startpoint;
+	state.starttli = backup_state->starttli;
 
 	/*
 	 * Once do_pg_backup_start has been called, ensure that any failure causes
@@ -317,14 +326,14 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 				bbsink_begin_archive(sink, "base.tar");
 
 				/* In the main tar, include the backup_label first... */
-				sendFileWithContent(sink, BACKUP_LABEL_FILE, labelfile->data,
-									&manifest);
+				sendFileWithContent(sink, BACKUP_LABEL_FILE,
+									backup_label->data, &manifest);
 
 				/* 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 +383,13 @@ 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_variables(backup_state, backup_label,
+									tablespace_map);
 	}
 	PG_END_ENSURE_ERROR_CLEANUP(do_pg_abort_backup, BoolGetDatum(false));
 
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 3dbfa6b593..5a47c5552b 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,9 @@ 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(BackupState *state, bool fast,
+							   List **tablespaces, StringInfo tablespace_map);
+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..5b43b7d1d7
--- /dev/null
+++ b/src/include/access/xlogbackup.h
@@ -0,0 +1,53 @@
+/*-------------------------------------------------------------------------
+ * xlogbackup.h
+ *
+ * Declarations for the backup related code.
+ *
+ * Copyright (c) 2022, PostgreSQL Global Development Group
+ *
+ * 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
+{
+	/* Following are the fields captured when the backup starts. */
+
+	/*
+	 * Null-terminated backup label name. If the label name is of size more
+	 * than MAXPGPATH, an error is emitted in do_pg_backup_start(). We do not
+	 * want to over-allocate memory in this structure, hence we truncate it to
+	 * MAXPGPATH + 2 bytes (one extra byte for representing that the name has
+	 * overflown, and another extra byte for null-termination).
+	 */
+	char            name[MAXPGPATH + 2];
+	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? */
+
+	/* Following are the fields captured during or after the backup stops. */
+
+	XLogRecPtr      stoppoint; /* backup stop WAL location */
+	TimeLineID	    stoptli;	/* backup stop TLI */
+	pg_time_t	    stoptime;	/* backup stop time */
+} BackupState;
+
+extern BackupState *allocate_backup_state(const char *name);
+extern void reset_backup_state(BackupState *state, const char *name);
+extern void deallocate_backup_variables(BackupState *state,
+										StringInfo backup_label,
+										StringInfo tablespace_map);
+extern void build_backup_content(BackupState *state,
+								 bool ishistoryfile,
+								 StringInfo str);
+#endif							/* XLOG_BACKUP_H */
-- 
2.34.1

From b20d5455f746ec8f2ca4bd325f8636aa2b991438 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 20 Sep 2022 10:44:42 +0000
Subject: [PATCH v9] Add error callbacks for deallocating backup related
 variables

This adds error callbacks for deallocating backup related
variables to save on some palloc-ed memory upon errors while
taking backup.
---
 src/backend/access/transam/xlogbackup.c | 22 ++++++++++
 src/backend/access/transam/xlogfuncs.c  | 54 +++++++++++++++++++++++++
 src/backend/backup/basebackup.c         | 14 +++++++
 src/backend/utils/error/elog.c          | 17 ++++++++
 src/include/access/xlogbackup.h         | 11 +++++
 src/include/utils/elog.h                |  1 +
 6 files changed, 119 insertions(+)

diff --git a/src/backend/access/transam/xlogbackup.c b/src/backend/access/transam/xlogbackup.c
index 38c37cc7db..39210af41b 100644
--- a/src/backend/access/transam/xlogbackup.c
+++ b/src/backend/access/transam/xlogbackup.c
@@ -92,6 +92,28 @@ deallocate_backup_variables(BackupState *state, StringInfo backup_label,
 		pfree(tablespace_map->data);
 }
 
+/*
+ * Erro callback for deallocating backup variables.
+ */
+void
+deallocate_backup_variables_err_cb(void *arg)
+{
+	BackupErrorInfo *errinfo = (BackupErrorInfo *) arg;
+
+	/*
+	 * Since error callbacks are called for elevel lesser than ERROR, we
+	 * proceed further only if elevel is ERROR or greater.
+	 */
+	if (geterrlevel() < ERROR)
+		return;
+
+	if (errinfo == NULL)
+		return; /* nothing to do */
+
+	deallocate_backup_variables(errinfo->state, errinfo->backup_label,
+								errinfo->tablespace_map);
+}
+
 /*
  * Construct backup_label file or history file contents
  *
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index e078579e6f..7d801aefc7 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -45,6 +45,32 @@
 static BackupState *backup_state = NULL;
 static StringInfo tablespace_map = NULL;
 
+/*
+ * Erro callback for deallocating and resetting backup variables.
+ */
+static void
+deallocate_and_reset_backup_variables_err_cb(void *arg)
+{
+	BackupErrorInfo *errinfo = (BackupErrorInfo *) arg;
+
+	/*
+	 * Since error callbacks are called for elevel lesser than ERROR, we
+	 * proceed further only if elevel is ERROR or greater.
+	 */
+	if (geterrlevel() < ERROR)
+		return;
+
+	if (errinfo == NULL)
+		return; /* nothing to do */
+
+	deallocate_backup_variables(errinfo->state, errinfo->backup_label,
+								errinfo->tablespace_map);
+
+	/* Reset the static variables. */
+	backup_state = NULL;
+	tablespace_map = NULL;
+}
+
 /*
  * pg_backup_start: start taking an on-line backup.
  *
@@ -61,6 +87,8 @@ pg_backup_start(PG_FUNCTION_ARGS)
 	char	   *backupidstr;
 	SessionBackupState status = get_backup_status();
 	MemoryContext oldcontext;
+	ErrorContextCallback errcallback;
+	BackupErrorInfo errinfo;
 
 	backupidstr = text_to_cstring(backupid);
 
@@ -93,9 +121,21 @@ pg_backup_start(PG_FUNCTION_ARGS)
 	tablespace_map = makeStringInfo();
 	MemoryContextSwitchTo(oldcontext);
 
+	/* Set up callback to clean up backup related variables */
+	errcallback.callback = deallocate_and_reset_backup_variables_err_cb;
+	errinfo.state = backup_state;
+	errinfo.backup_label = NULL;
+	errinfo.tablespace_map = tablespace_map;
+	errcallback.arg = (void *) &errinfo;
+	errcallback.previous = error_context_stack;
+	error_context_stack = &errcallback;
+
 	register_persistent_abort_backup_handler();
 	do_pg_backup_start(backup_state, fast, NULL, tablespace_map);
 
+	/* Pop the error context stack */
+	error_context_stack = errcallback.previous;
+
 	PG_RETURN_LSN(backup_state->startpoint);
 }
 
@@ -123,6 +163,8 @@ pg_backup_stop(PG_FUNCTION_ARGS)
 	bool		waitforarchive = PG_GETARG_BOOL(0);
 	StringInfo	backup_label;
 	SessionBackupState status = get_backup_status();
+	ErrorContextCallback errcallback;
+	BackupErrorInfo errinfo;
 
 	/* Initialize attributes information in the tuple descriptor */
 	if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
@@ -141,12 +183,24 @@ pg_backup_stop(PG_FUNCTION_ARGS)
 	Assert(backup_state != NULL);
 	Assert(tablespace_map != NULL);
 
+	/* Set up callback to clean up backup related variables */
+	errcallback.callback = deallocate_and_reset_backup_variables_err_cb;
+	errinfo.state = backup_state;
+	errinfo.backup_label = NULL;
+	errinfo.tablespace_map = tablespace_map;
+	errcallback.arg = (void *) &errinfo;
+	errcallback.previous = error_context_stack;
+	error_context_stack = &errcallback;
+
 	/*
 	 * Stop the backup. Return a copy of the backup label and tablespace map
 	 * so they can be written to disk by the caller.
 	 */
 	do_pg_backup_stop(backup_state, waitforarchive);
 
+	/* Pop the error context stack */
+	error_context_stack = errcallback.previous;
+
 	/*
 	 * Construct backup_label contents. Note that the backup_label doesn't have
 	 * to be in TopMemoryContext unlike backup_state and tablespace_map as it
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 7ba4f340bc..3edbe4c460 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -236,6 +236,8 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 	BackupState *backup_state;
 	StringInfo	backup_label;
 	StringInfo	tablespace_map;
+	ErrorContextCallback errcallback;
+	BackupErrorInfo errinfo;
 
 	/* Initial backup state, insofar as we know it now. */
 	state.tablespaces = NIL;
@@ -260,6 +262,15 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 	backup_label = makeStringInfo();
 	tablespace_map = makeStringInfo();
 
+	/* Set up callback to clean up backup related variables */
+	errcallback.callback = deallocate_backup_variables_err_cb;
+	errinfo.state = backup_state;
+	errinfo.backup_label = backup_label;
+	errinfo.tablespace_map = tablespace_map;
+	errcallback.arg = (void *) &errinfo;
+	errcallback.previous = error_context_stack;
+	error_context_stack = &errcallback;
+
 	basebackup_progress_wait_checkpoint();
 	do_pg_backup_start(backup_state, opt->fastcheckpoint,
 					   &state.tablespaces, tablespace_map);
@@ -663,6 +674,9 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
 	/* clean up the resource owner we created */
 	WalSndResourceCleanup(true);
 
+	/* Pop the error context stack */
+	error_context_stack = errcallback.previous;
+
 	basebackup_progress_done();
 }
 
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 96c694da8f..d2868648db 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1404,6 +1404,23 @@ geterrcode(void)
 	return edata->sqlerrcode;
 }
 
+/*
+ * geterrlevel --- return the currently error level
+ *
+ * This is only intended for use in error callback subroutines, since there
+ * is no other place outside elog.c where the concept is meaningful.
+ */
+int
+geterrlevel(void)
+{
+	ErrorData  *edata = &errordata[errordata_stack_depth];
+
+	/* we don't bother incrementing recursion_depth */
+	CHECK_STACK_DEPTH();
+
+	return edata->elevel;
+}
+
 /*
  * geterrposition --- return the currently set error position (0 if none)
  *
diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h
index 5b43b7d1d7..e98fe6132c 100644
--- a/src/include/access/xlogbackup.h
+++ b/src/include/access/xlogbackup.h
@@ -42,6 +42,16 @@ typedef struct BackupState
 	pg_time_t	    stoptime;	/* backup stop time */
 } BackupState;
 
+/*
+ * Structure to hold input parameters for backup error callback.
+ */
+typedef struct BackupErrorInfo
+{
+	BackupState *state;
+	StringInfo	backup_label;
+	StringInfo	tablespace_map;
+} BackupErrorInfo;
+
 extern BackupState *allocate_backup_state(const char *name);
 extern void reset_backup_state(BackupState *state, const char *name);
 extern void deallocate_backup_variables(BackupState *state,
@@ -50,4 +60,5 @@ extern void deallocate_backup_variables(BackupState *state,
 extern void build_backup_content(BackupState *state,
 								 bool ishistoryfile,
 								 StringInfo str);
+extern void deallocate_backup_variables_err_cb(void *arg);
 #endif							/* XLOG_BACKUP_H */
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4dd9658a3c..a8955f9572 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -222,6 +222,7 @@ extern int	internalerrquery(const char *query);
 extern int	err_generic_string(int field, const char *str);
 
 extern int	geterrcode(void);
+extern int	geterrlevel(void);
 extern int	geterrposition(void);
 extern int	getinternalerrposition(void);
 
-- 
2.34.1

Reply via email to