Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file - Mailing list pgsql-hackers

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