From fa6156f8cab27be4f743e508ec3d5ed6cb200b23 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Fri, 14 Oct 2022 12:38:23 +0000
Subject: [PATCH v6] Use do_pg_abort_backup() consistently across the backup
 code

Backup code uses another abort callback pg_backup_start_callback()
when it has do_pg_abort_backup(). These two are before_shmem_exit()
callbacks more or less do the same thing. The only difference
between them is resetting sessionBackupState to SESSION_BACKUP_NONE.
Actually, it is safe for us to reset sessionBackupState on aborts.
This leads us to remove the pg_backup_start_callback() and use
do_pg_abort_backup() consistently across.

Author: Bharath Rupireddy per suggestion from Alvaro Herrera.
Discussion: https://www.postgresql.org/message-id/20221013111330.564fk5tkwe3ha77l%40alvherre.pgsql
---
 src/backend/access/transam/xlog.c | 40 +++++++++----------------------
 1 file changed, 11 insertions(+), 29 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 27085b15a8..0b7a0b5ac0 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -679,8 +679,6 @@ static void ReadControlFile(void);
 static void UpdateControlFile(void);
 static char *str_time(pg_time_t tnow);
 
-static void pg_backup_start_callback(int code, Datum arg);
-
 static int	get_sync_bit(int method);
 
 static void CopyXLogRecordToWAL(int write_len, bool isLogSwitch,
@@ -8321,7 +8319,7 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
 	WALInsertLockRelease();
 
 	/* Ensure we release forcePageWrites if fail below */
-	PG_ENSURE_ERROR_CLEANUP(pg_backup_start_callback, (Datum) 0);
+	PG_ENSURE_ERROR_CLEANUP(do_pg_abort_backup, DatumGetBool(false));
 	{
 		bool		gotUniqueStartpoint = false;
 		DIR		   *tblspcdir;
@@ -8531,7 +8529,7 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
 
 		state->starttime = (pg_time_t) time(NULL);
 	}
-	PG_END_ENSURE_ERROR_CLEANUP(pg_backup_start_callback, (Datum) 0);
+	PG_END_ENSURE_ERROR_CLEANUP(do_pg_abort_backup, DatumGetBool(false));
 
 	state->started_in_recovery = backup_started_in_recovery;
 
@@ -8541,23 +8539,6 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
 	sessionBackupState = SESSION_BACKUP_RUNNING;
 }
 
-/* Error cleanup callback for pg_backup_start */
-static void
-pg_backup_start_callback(int code, Datum arg)
-{
-	/* Update backup counters and forcePageWrites on failure */
-	WALInsertLockAcquireExclusive();
-
-	Assert(XLogCtl->Insert.runningBackups > 0);
-	XLogCtl->Insert.runningBackups--;
-
-	if (XLogCtl->Insert.runningBackups == 0)
-	{
-		XLogCtl->Insert.forcePageWrites = false;
-	}
-	WALInsertLockRelease();
-}
-
 /*
  * Utility routine to fetch the session-level status of a backup running.
  */
@@ -8850,8 +8831,9 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
  * system out of backup mode, thus making it a lot more safe to call from
  * an error handler.
  *
- * The caller can pass 'arg' as 'true' or 'false' to control whether a warning
- * is emitted.
+ * Pass 'arg' as 'true' so that the function can quickly exit if the function
+ * is called before sessionBackupState and backup variables in XLogCtl are set.
+ * Otherwise pass 'false' so that the function can reset them up.
  *
  * NB: This gets used as a before_shmem_exit handler, hence the odd-looking
  * signature.
@@ -8859,12 +8841,13 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
 void
 do_pg_abort_backup(int code, Datum arg)
 {
-	bool		emit_warning = DatumGetBool(arg);
+	bool	can_quickly_exit = DatumGetBool(arg);
 
 	/*
-	 * Quick exit if session does not have a running backup.
+	 * Quick exit if we're told to and session does not have a running backup.
 	 */
-	if (sessionBackupState != SESSION_BACKUP_RUNNING)
+	if (can_quickly_exit &&
+		sessionBackupState != SESSION_BACKUP_RUNNING)
 		return;
 
 	WALInsertLockAcquireExclusive();
@@ -8879,9 +8862,8 @@ do_pg_abort_backup(int code, Datum arg)
 	sessionBackupState = SESSION_BACKUP_NONE;
 	WALInsertLockRelease();
 
-	if (emit_warning)
-		ereport(WARNING,
-				(errmsg("aborting backup due to backend exiting before pg_backup_stop was called")));
+	ereport(WARNING,
+			(errmsg("aborting a running backup before exiting the backend")));
 }
 
 /*
-- 
2.34.1