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

I've attached an updated patch.

On Fri, Feb 18, 2022 at 10:48:10PM +0530, Ashutosh Sharma wrote:
> +    * 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.

Fixed this.

> -    * 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:

Fixed this.

> 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.

I didn't follow through with this change.  I only saw a handful of lines
that looked similar, and AFAICT we'd need an extra branch for cleaning up
the session-level lock since do_pg_abort_backup() doesn't.

> +# 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.

IIUC this test relied on the following code to handle the bogus file:

            /*
             * Terminate exclusive backup mode to avoid recovery after a clean
             * fast shutdown.  Since an exclusive backup can only be taken
             * during normal running (and not, for example, while running
             * under Hot Standby) it only makes sense to do this if we reached
             * normal running. If we're still in recovery, the backup file is
             * one we're recovering *from*, and we must keep it around so that
             * recovery restarts from the right place.
             */
            if (ReachedNormalRunning)
                CancelBackup();

The attached patch removes this code.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com