|From:||Nathan Bossart <nathandbossart(at)gmail(dot)com>|
|To:||Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>|
|Cc:||"Bossart, Nathan" <bossartn(at)amazon(dot)com>, David Steele <david(at)pgmasters(dot)net>, Euler Taveira <euler(at)eulerto(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Stephen Frost <sfrost(at)snowman(dot)net>, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>, Michael Paquier <michael(at)paquier(dot)xyz>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, SATYANARAYANA NARLAPURAM <satyanarlapuram(at)gmail(dot)com>|
|Subject:||Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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.
> - * 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:
> 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
> + 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.
The attached patch removes this code.
Amazon Web Services: https://aws.amazon.com
|Next Message||Andres Freund||2022-02-21 17:31:09||Re: making pg_regress less noisy by removing boilerplate|
|Previous Message||Tom Lane||2022-02-21 17:11:16||Re: show schema.collate in explain(verbose on)|