Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file

Some review comments on the latest version:

+ * runningBackups is a counter indicating the number of backups currently in
+ * progress. forcePageWrites is set to true when either of these is
+ * non-zero. lastBackupStart is the latest checkpoint redo location used as
+ * a starting point for an online backup.
*/
- ExclusiveBackupState exclusiveBackupState;
- int nonExclusiveBackups;

What do you mean by "either of these is non-zero ''. Earlier we used
to set forcePageWrites in case of both exclusive and non-exclusive
backups, but we have just one type of backup now.

==

- * OK to update backup counters, forcePageWrites and session-level lock.
+ * OK to update backup counters and forcePageWrites.
*

We still update the status of session-level lock so I don't think we
should update the above comment. See below code:

if (XLogCtl->Insert.runningBackups == 0)
{
XLogCtl->Insert.forcePageWrites = false;
}

/*
* Clean up session-level lock.
*
* You might think that WALInsertLockRelease() can be called before
* cleaning up session-level lock because session-level lock doesn't need
* to be protected with WAL insertion lock. But since
* CHECK_FOR_INTERRUPTS() can occur in it, session-level lock must be
* cleaned up before it.
*/
sessionBackupState = SESSION_BACKUP_NONE;

WALInsertLockRelease();

==

@@ -8993,18 +8686,16 @@ do_pg_abort_backup(int code, Datum arg)
bool emit_warning = DatumGetBool(arg);

/*
- * Quick exit if session is not keeping around a non-exclusive backup
- * already started.
+ * Quick exit if session does not have a running backup.
*/
- if (sessionBackupState != SESSION_BACKUP_NON_EXCLUSIVE)
+ if (sessionBackupState != SESSION_BACKUP_RUNNING)
return;

WALInsertLockAcquireExclusive();
- Assert(XLogCtl->Insert.nonExclusiveBackups > 0);
- XLogCtl->Insert.nonExclusiveBackups--;
+ Assert(XLogCtl->Insert.runningBackups > 0);
+ XLogCtl->Insert.runningBackups--;

- if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_NONE &&
- XLogCtl->Insert.nonExclusiveBackups == 0)
+ if (XLogCtl->Insert.runningBackups == 0)
{
XLogCtl->Insert.forcePageWrites = false;
}

I think we have a lot of common code in do_pg_abort_backup() and
pg_do_stop_backup(). So why not have a common function that can be
called from both these functions.

==

+# Now delete the bogus backup_label file since it will interfere with startup
+unlink("$pgdata/backup_label")
+ or BAIL_OUT("unable to unlink $pgdata/backup_label");
+

Why do we need this additional change? Earlier this was not required.

--
With Regards,
Ashutosh Sharma.

On Thu, Feb 17, 2022 at 6:41 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> Here is a rebased patch.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com