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 <[email protected]> wrote:
>
> Here is a rebased patch.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
