Fix recently added incorrect assertion

alvherre · alvherre · commit 8328a15f8f95 · 2022-10-24T12:02:33.000+02:00
Commit df3737a added an incorrect assertion about the preconditions for invoking the backup cleanup callback: it misfires at session end in case a backup completes successfully. Fix it, using coding from Michaël Paquier. Also add some tests for the various cases. Reported by Kyotaro Horiguchi <horikyota.ntt@gmail.com> Discussion: https://postgr.es/m/20221021.161038.1277961198945653224.horikyota.ntt@gmail.com
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
@@ -8841,9 +8841,8 @@ do_pg_abort_backup(int code, Datum arg)
 {
 	bool		during_backup_start = DatumGetBool(arg);
 
-	/* Only one of these conditions can be true */
-	Assert(during_backup_start ^
-		   (sessionBackupState == SESSION_BACKUP_RUNNING));
+	/* If called during backup start, there shouldn't be one already running */
+	Assert(!during_backup_start || sessionBackupState == SESSION_BACKUP_NONE);
 
 	if (during_backup_start || sessionBackupState != SESSION_BACKUP_NONE)
 	{
diff --git a/src/test/recovery/t/020_archive_status.pl b/src/test/recovery/t/020_archive_status.pl
@@ -248,4 +248,18 @@
 ok( $logfile =~ qr/archiver process shutting down/,
 	'check shutdown callback of shell archive module');
 
+# Test that we can enter and leave backup mode without crashes
+my ($stderr, $cmdret);
+$cmdret = $primary->psql(
+	'postgres',
+	"SELECT pg_backup_start('onebackup'); "
+	  . "SELECT pg_backup_stop();"
+	  . "SELECT pg_backup_start(repeat('x', 1026))",
+	stderr => \$stderr);
+is($cmdret, 3, "psql fails correctly");
+like($stderr, qr/backup label too long/, "pg_backup_start fails gracefully");
+$primary->safe_psql('postgres',
+	"SELECT pg_backup_start('onebackup'); SELECT pg_backup_stop();");
+$primary->safe_psql('postgres', "SELECT pg_backup_start('twobackup')");
+
 done_testing();

Original file line number	Diff line number	Diff line change
`@@ -8841,9 +8841,8 @@ do_pg_abort_backup(int code, Datum arg)`
`8841`	`8841`	`{`
`8842`	`8842`	`bool during_backup_start = DatumGetBool(arg);`
`8843`	`8843`
`8844`		`- /* Only one of these conditions can be true */`
`8845`		`- Assert(during_backup_start ^`
`8846`		`- (sessionBackupState == SESSION_BACKUP_RUNNING));`
	`8844`	`+ /* If called during backup start, there shouldn't be one already running */`
	`8845`	`+ Assert(!during_backup_start \|\| sessionBackupState == SESSION_BACKUP_NONE);`
`8847`	`8846`
`8848`	`8847`	`if (during_backup_start \|\| sessionBackupState != SESSION_BACKUP_NONE)`
`8849`	`8848`	`{`