public inbox for [email protected]help / color / mirror / Atom feed
Postgres restart in the middle of exclusive backup and the presence of backup_label file 28+ messages / 9 participants [nested] [flat]
* Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-24 22:12 SATYANARAYANA NARLAPURAM <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: SATYANARAYANA NARLAPURAM @ 2021-11-24 22:12 UTC (permalink / raw) To: pgsql-hackers Hi Hackers, While an exclusive backup is in progress if Postgres restarts, postgres runs the recovery from the checkpoint identified by the label file instead of the control file. This can cause long recovery or even sometimes fail to recover as the WAL records corresponding to that checkpoint location are removed. I can write a layer in my control plane to remove the backup_label file when I know the server is not in restore from the base backup but I don't see a reason why everyone has to repeat this step. Am I missing something? If there are no standby.signal or recovery.signal, what is the use case of honoring backup_label file? Even when they exist, for a long running recovery, should we honor the backup_label file as the majority of the WAL already applied? It does slow down the recovery on restart right as it has to start all the way from the beginning? Thanks, Satya ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-24 22:45 Michael Paquier <[email protected]> parent: SATYANARAYANA NARLAPURAM <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Michael Paquier @ 2021-11-24 22:45 UTC (permalink / raw) To: SATYANARAYANA NARLAPURAM <[email protected]>; +Cc: pgsql-hackers On Wed, Nov 24, 2021 at 02:12:19PM -0800, SATYANARAYANA NARLAPURAM wrote: > While an exclusive backup is in progress if Postgres restarts, postgres > runs the recovery from the checkpoint identified by the label file instead > of the control file. This can cause long recovery or even sometimes fail to > recover as the WAL records corresponding to that checkpoint location are > removed. I can write a layer in my control plane to remove the backup_label > file when I know the server is not in restore from the base backup but I > don't see a reason why everyone has to repeat this step. Am I missing > something? This is a known issue with exclusive backups, which is a reason why non-exclusive backups have been implemented. pg_basebackup does that, and using "false" as the third argument of pg_start_backup() would have the same effect. So I would recommend to switch to that. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-26 02:19 SATYANARAYANA NARLAPURAM <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: SATYANARAYANA NARLAPURAM @ 2021-11-26 02:19 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers Thanks Michael! This is a known issue with exclusive backups, which is a reason why > non-exclusive backups have been implemented. pg_basebackup does that, > and using "false" as the third argument of pg_start_backup() would > have the same effect. So I would recommend to switch to that. > Is there a plan in place to remove the exclusive backup option from the core in PG 15/16? If we are keeping it then why not make it better? ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-26 05:57 Michael Paquier <[email protected]> parent: SATYANARAYANA NARLAPURAM <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Michael Paquier @ 2021-11-26 05:57 UTC (permalink / raw) To: SATYANARAYANA NARLAPURAM <[email protected]>; +Cc: pgsql-hackers On Thu, Nov 25, 2021 at 06:19:03PM -0800, SATYANARAYANA NARLAPURAM wrote: > Is there a plan in place to remove the exclusive backup option from the > core in PG 15/16? This was discussed, but removing it could also harm users relying on it. Perhaps it could be revisited, but I am not sure if this is worth worrying about either. > If we are keeping it then why not make it better? Well, non-exclusive backups are better by design in many aspects, so I don't quite see the point in spending time on something that has more limitations than what's already in place. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../YaB3Oj4GfPOZ%[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-26 15:31 Tom Lane <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 2 replies; 28+ messages in thread From: Tom Lane @ 2021-11-26 15:31 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: SATYANARAYANA NARLAPURAM <[email protected]>; pgsql-hackers Michael Paquier <[email protected]> writes: > On Thu, Nov 25, 2021 at 06:19:03PM -0800, SATYANARAYANA NARLAPURAM wrote: >> If we are keeping it then why not make it better? > Well, non-exclusive backups are better by design in many aspects, so I > don't quite see the point in spending time on something that has more > limitations than what's already in place. IMO the main reason for keeping it is backwards compatibility for users who have a satisfactory backup arrangement using it. That same argument implies that we shouldn't change how it works (at least, not very much). regards, tom lane ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-29 18:25 Bossart, Nathan <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 28+ messages in thread From: Bossart, Nathan @ 2021-11-29 18:25 UTC (permalink / raw) To: Tom Lane <[email protected]>; Michael Paquier <[email protected]>; +Cc: SATYANARAYANA NARLAPURAM <[email protected]>; pgsql-hackers On 11/26/21, 7:33 AM, "Tom Lane" <[email protected]> wrote: > Michael Paquier <[email protected]> writes: >> On Thu, Nov 25, 2021 at 06:19:03PM -0800, SATYANARAYANA NARLAPURAM wrote: >>> If we are keeping it then why not make it better? > >> Well, non-exclusive backups are better by design in many aspects, so I >> don't quite see the point in spending time on something that has more >> limitations than what's already in place. > > IMO the main reason for keeping it is backwards compatibility for users > who have a satisfactory backup arrangement using it. That same argument > implies that we shouldn't change how it works (at least, not very much). The issues with exclusive backups seem to be fairly well-documented (e.g., c900c15), but perhaps there should also be a note in the "Backup Control Functions" table [0]. Nathan [0] https://www.postgresql.org/docs/devel/functions-admin.html#FUNCTIONS-ADMIN-BACKUP ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-30 14:20 Stephen Frost <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 28+ messages in thread From: Stephen Frost @ 2021-11-30 14:20 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; SATYANARAYANA NARLAPURAM <[email protected]>; pgsql-hackers Greetings, * Tom Lane ([email protected]) wrote: > Michael Paquier <[email protected]> writes: > > On Thu, Nov 25, 2021 at 06:19:03PM -0800, SATYANARAYANA NARLAPURAM wrote: > >> If we are keeping it then why not make it better? > > > Well, non-exclusive backups are better by design in many aspects, so I > > don't quite see the point in spending time on something that has more > > limitations than what's already in place. > > IMO the main reason for keeping it is backwards compatibility for users > who have a satisfactory backup arrangement using it. That same argument > implies that we shouldn't change how it works (at least, not very much). There isn't a satisfactory backup approach using it specifically because of this issue, hence why we should remove it to make it so users don't run into this. Would also simplify the documentation around the low level backup API, which would be a very good thing. Right now, making improvements in that area is very challenging even if all you want to do is improve the documentation around the non-exclusive API. We dealt with this as best as one could in pgbackrest for PG versions prior to when non-exclusive backup was added- which is to remove the backup_label file as soon as possible and then put it back right before you call pg_stop_backup() (since it'll complain otherwise). Not a perfect answer though and a risk still exists there of a failed restart happening. Of course, for versions which support non-exclusive backup, we use that to avoid this issue. We also extensively changed how restore works a couple releases ago and while there was some noise about it, it certainly wasn't all that bad. I don't find the reasons brought up to continue to support exclusive backup to be at all compelling and the lack of huge issues with the new way restore works to make it abundently clear that we can, in fact, remove exclusive backup in a major version change without the world coming down. Thanks, Stephen Attachments: [application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-30 16:47 Laurenz Albe <[email protected]> parent: Stephen Frost <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Laurenz Albe @ 2021-11-30 16:47 UTC (permalink / raw) To: Stephen Frost <[email protected]>; Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; SATYANARAYANA NARLAPURAM <[email protected]>; pgsql-hackers On Tue, 2021-11-30 at 09:20 -0500, Stephen Frost wrote: > Greetings, > > * Tom Lane ([email protected]) wrote: > > Michael Paquier <[email protected]> writes: > > > On Thu, Nov 25, 2021 at 06:19:03PM -0800, SATYANARAYANA NARLAPURAM wrote: > > > > If we are keeping it then why not make it better? > > > > > Well, non-exclusive backups are better by design in many aspects, so I > > > don't quite see the point in spending time on something that has more > > > limitations than what's already in place. > > > > IMO the main reason for keeping it is backwards compatibility for users > > who have a satisfactory backup arrangement using it. That same argument > > implies that we shouldn't change how it works (at least, not very much). > > There isn't a satisfactory backup approach using it specifically because > of this issue, hence why we should remove it to make it so users don't > run into this. There is a satisfactory approach, as long as you are satisfied with manually restarting the server if it crashed during a backup. > I don't find the reasons brought up to continue to support exclusive > backup to be at all compelling and the lack of huge issues with the new > way restore works to make it abundently clear that we can, in fact, > remove exclusive backup in a major version change without the world > coming down. I guess the lack of hue and cry was at least to a certain extent because the exclusive backup API was deprecated, but not removed. Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-30 17:48 Stephen Frost <[email protected]> parent: Laurenz Albe <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Stephen Frost @ 2021-11-30 17:48 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]>; Tom Lane <[email protected]> Greetings, On Tue, Nov 30, 2021 at 11:47 Laurenz Albe <[email protected]> wrote: > On Tue, 2021-11-30 at 09:20 -0500, Stephen Frost wrote: > > Greetings, > > > > * Tom Lane ([email protected]) wrote: > > > Michael Paquier <[email protected]> writes: > > > > On Thu, Nov 25, 2021 at 06:19:03PM -0800, SATYANARAYANA NARLAPURAM > wrote: > > > > > If we are keeping it then why not make it better? > > > > > > > Well, non-exclusive backups are better by design in many aspects, so > I > > > > don't quite see the point in spending time on something that has more > > > > limitations than what's already in place. > > > > > > IMO the main reason for keeping it is backwards compatibility for users > > > who have a satisfactory backup arrangement using it. That same > argument > > > implies that we shouldn't change how it works (at least, not very > much). > > > > There isn't a satisfactory backup approach using it specifically because > > of this issue, hence why we should remove it to make it so users don't > > run into this. > > There is a satisfactory approach, as long as you are satisfied with > manually restarting the server if it crashed during a backup. I disagree that that’s a satisfactory approach. It certainly wasn’t intended or documented as part of the original feature and therefore to call it satisfactory strikes me quite strongly as revisionist history. > I don't find the reasons brought up to continue to support exclusive > > backup to be at all compelling and the lack of huge issues with the new > > way restore works to make it abundently clear that we can, in fact, > > remove exclusive backup in a major version change without the world > > coming down. > > I guess the lack of hue and cry was at least to a certain extent because > the exclusive backup API was deprecated, but not removed. These comments were in reference to the restore API, which was quite changed (new special files that have to be touched, removing of recovery.conf, options moved to postgresql.conf/.auto, etc). So, no. Thanks, Stephen > ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-30 21:56 Bossart, Nathan <[email protected]> parent: Stephen Frost <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Bossart, Nathan @ 2021-11-30 21:56 UTC (permalink / raw) To: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]>; Tom Lane <[email protected]> On 11/30/21, 9:51 AM, "Stephen Frost" <[email protected]> wrote: > I disagree that that’s a satisfactory approach. It certainly wasn’t > intended or documented as part of the original feature and therefore > to call it satisfactory strikes me quite strongly as revisionist > history. It looks like the exclusive way has been marked deprecated in all supported versions along with a note that it will eventually be removed. If it's not going to be removed out of fear of breaking backward compatibility, I think the documentation should be updated to say that. However, unless there is something that is preventing users from switching to the non-exclusive approach, I think it is reasonable to begin thinking about removing it. Nathan ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-30 22:26 Tom Lane <[email protected]> parent: Bossart, Nathan <[email protected]> 0 siblings, 3 replies; 28+ messages in thread From: Tom Lane @ 2021-11-30 22:26 UTC (permalink / raw) To: Bossart, Nathan <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> "Bossart, Nathan" <[email protected]> writes: > It looks like the exclusive way has been marked deprecated in all > supported versions along with a note that it will eventually be > removed. If it's not going to be removed out of fear of breaking > backward compatibility, I think the documentation should be updated to > say that. However, unless there is something that is preventing users > from switching to the non-exclusive approach, I think it is reasonable > to begin thinking about removing it. If we're willing to outright remove it, I don't have any great objection. My original two cents was that we shouldn't put effort into improving it; but removing it isn't that. regards, tom lane ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-30 22:50 Bossart, Nathan <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 0 replies; 28+ messages in thread From: Bossart, Nathan @ 2021-11-30 22:50 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 11/30/21, 2:27 PM, "Tom Lane" <[email protected]> wrote: > If we're willing to outright remove it, I don't have any great objection. > My original two cents was that we shouldn't put effort into improving it; > but removing it isn't that. I might try to put a patch together for the January commitfest, given there is enough support. Nathan ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-30 22:58 David Steele <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 2 replies; 28+ messages in thread From: David Steele @ 2021-11-30 22:58 UTC (permalink / raw) To: Tom Lane <[email protected]>; Bossart, Nathan <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 11/30/21 17:26, Tom Lane wrote: > "Bossart, Nathan" <[email protected]> writes: >> It looks like the exclusive way has been marked deprecated in all >> supported versions along with a note that it will eventually be >> removed. If it's not going to be removed out of fear of breaking >> backward compatibility, I think the documentation should be updated to >> say that. However, unless there is something that is preventing users >> from switching to the non-exclusive approach, I think it is reasonable >> to begin thinking about removing it. > > If we're willing to outright remove it, I don't have any great objection. > My original two cents was that we shouldn't put effort into improving it; > but removing it isn't that. The main objections as I recall are that it is much harder for simple backup scripts and commercial backup integrations to hold a connection to postgres open and write the backup label separately into the backup. As Stephen noted, working in this area is much harder (even in the docs) due to the need to keep both methods working. When I removed exclusive backup it didn't break any tests, other than one that needed to generate a corrupt backup, so we have virtually no coverage for that method. I did figure out how to keep the safe part of exclusive backup (not having to maintain a connection) while removing the dangerous part (writing backup_label into PGDATA), but it was a substantial amount of work and I felt that it had little chance of being committed. Attaching the thread [1] that I started with a patch to remove exclusive backup for reference. -- [1] https://www.postgresql.org/message-id/flat/ac7339ca-3718-3c93-929f-99e725d1172c%40pgmasters.net ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-11-30 23:31 Bossart, Nathan <[email protected]> parent: David Steele <[email protected]> 1 sibling, 1 reply; 28+ messages in thread From: Bossart, Nathan @ 2021-11-30 23:31 UTC (permalink / raw) To: David Steele <[email protected]>; Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 11/30/21, 2:58 PM, "David Steele" <[email protected]> wrote: > I did figure out how to keep the safe part of exclusive backup (not > having to maintain a connection) while removing the dangerous part > (writing backup_label into PGDATA), but it was a substantial amount of > work and I felt that it had little chance of being committed. Do you think it's still worth trying to make it safe, or do you think we should just remove exclusive mode completely? > Attaching the thread [1] that I started with a patch to remove exclusive > backup for reference. Ah, good, some light reading. :) Nathan ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-12-01 00:54 Michael Paquier <[email protected]> parent: David Steele <[email protected]> 1 sibling, 2 replies; 28+ messages in thread From: Michael Paquier @ 2021-12-01 00:54 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: Tom Lane <[email protected]>; Bossart, Nathan <[email protected]>; Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On Tue, Nov 30, 2021 at 05:58:15PM -0500, David Steele wrote: > The main objections as I recall are that it is much harder for simple backup > scripts and commercial backup integrations to hold a connection to postgres > open and write the backup label separately into the backup. I don't quite understand why this argument would not hold even today, even if I'd like to think that more people are using pg_basebackup. > I did figure out how to keep the safe part of exclusive backup (not having > to maintain a connection) while removing the dangerous part (writing > backup_label into PGDATA), but it was a substantial amount of work and I > felt that it had little chance of being committed. Which was, I guess, done by storing the backup_label contents within a file different than backup_label, still maintained in the main data folder to ensure that it gets included in the backup? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-12-01 01:56 SATYANARAYANA NARLAPURAM <[email protected]> parent: Michael Paquier <[email protected]> 1 sibling, 0 replies; 28+ messages in thread From: SATYANARAYANA NARLAPURAM @ 2021-12-01 01:56 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: David Steele <[email protected]>; Tom Lane <[email protected]>; Bossart, Nathan <[email protected]>; Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; pgsql-hackers On Tue, Nov 30, 2021 at 4:54 PM Michael Paquier <[email protected]> wrote: > On Tue, Nov 30, 2021 at 05:58:15PM -0500, David Steele wrote: > > The main objections as I recall are that it is much harder for simple > backup > > scripts and commercial backup integrations to hold a connection to > postgres > > open and write the backup label separately into the backup. > > I don't quite understand why this argument would not hold even today, > even if I'd like to think that more people are using pg_basebackup. > > > I did figure out how to keep the safe part of exclusive backup (not > having > > to maintain a connection) while removing the dangerous part (writing > > backup_label into PGDATA), but it was a substantial amount of work and I > > felt that it had little chance of being committed. > > Which was, I guess, done by storing the backup_label contents within a > file different than backup_label, still maintained in the main data > folder to ensure that it gets included in the backup? > Non-exclusive backup has significant advantages over exclusive backups but would like to add a few comments on the simplicity of exclusive backups - 1/ It is not uncommon nowadays to take a snapshot based backup. Exclusive backup simplifies this story as the backup label file is part of the snapshot. Otherwise, one needs to store it somewhere outside as snapshot metadata and copy this file over during restore (after creating a disk from the snapshot) to the data directory. Typical steps included are 1/ start pg_base_backup 2/ Take disk snapshot 3/ pg_stop_backup() 4/ Mark snapshot as consistent and add some create time metadata. 2/ Control plane code responsible for taking backups is simpler with exclusive backups than non-exclusive as it doesn't maintain a connection to the server, particularly when that orchestration is outside the machine the Postgres server is running on. IMHO, we should either remove the support for it or improve it but not leave it hanging there. ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-12-01 16:00 David Steele <[email protected]> parent: Michael Paquier <[email protected]> 1 sibling, 0 replies; 28+ messages in thread From: David Steele @ 2021-12-01 16:00 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; Bossart, Nathan <[email protected]>; Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 11/30/21 19:54, Michael Paquier wrote: > On Tue, Nov 30, 2021 at 05:58:15PM -0500, David Steele wrote: >> I did figure out how to keep the safe part of exclusive backup (not having >> to maintain a connection) while removing the dangerous part (writing >> backup_label into PGDATA), but it was a substantial amount of work and I >> felt that it had little chance of being committed. > > Which was, I guess, done by storing the backup_label contents within a > file different than backup_label, still maintained in the main data > folder to ensure that it gets included in the backup? That, or emit it from pg_start_backup() so the user can write it wherever they please. That would include writing it into PGDATA if they really wanted to, but that would be on them and the default behavior would be safe. The problem with this is if the user does not rename/supply backup_label on restore then they will get corruption and not know it. Here's another idea. Since the contents of pg_wal are not supposed to be copied, we could add a file there to indicate that the cluster should remove backup_label on restart. Our instructions also say to remove the contents of pg_wal on restore if they were originally copied, so hopefully one of the two would happen. But, again, if they fail to follow the directions it would lead to corruption. Order would be important here. When starting the backup the proper order would be to write pg_wal/backup_in_progress and then backup_label. When stopping the backup they would be removed in the reverse order. On a restart if both are present then delete both in the correct order and start crash recovery using the info in pg_control. If only backup_label is present then go into recovery using the info from backup_label. It's possible for pg_wal/backup_in_process to be present by itself if the server crashes after deleting backup_label but before deleting pg_wal/backup_in_progress. In that case the server should simply remove it on start and go into crash recovery using the info from pg_control. The advantage of this idea is that it does not change the current instructions as far as I can see. If the user is already following them, they'll be fine. If they are not, then they'll need to start doing so. Of course, none of this affects users who are using non-exclusive backup, which I do hope covers the majority by now. Thoughts? Regards, -David ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-12-01 16:14 Andrew Dunstan <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 0 replies; 28+ messages in thread From: Andrew Dunstan @ 2021-12-01 16:14 UTC (permalink / raw) To: Tom Lane <[email protected]>; Bossart, Nathan <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 11/30/21 17:26, Tom Lane wrote: > "Bossart, Nathan" <[email protected]> writes: >> It looks like the exclusive way has been marked deprecated in all >> supported versions along with a note that it will eventually be >> removed. If it's not going to be removed out of fear of breaking >> backward compatibility, I think the documentation should be updated to >> say that. However, unless there is something that is preventing users >> from switching to the non-exclusive approach, I think it is reasonable >> to begin thinking about removing it. > If we're willing to outright remove it, I don't have any great objection. > My original two cents was that we shouldn't put effort into improving it; > but removing it isn't that. > > +1 Let's just remove it. We already know it's a footgun, and there's been plenty of warning. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-12-01 16:27 David Steele <[email protected]> parent: Bossart, Nathan <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: David Steele @ 2021-12-01 16:27 UTC (permalink / raw) To: Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 11/30/21 18:31, Bossart, Nathan wrote: > On 11/30/21, 2:58 PM, "David Steele" <[email protected]> wrote: >> I did figure out how to keep the safe part of exclusive backup (not >> having to maintain a connection) while removing the dangerous part >> (writing backup_label into PGDATA), but it was a substantial amount of >> work and I felt that it had little chance of being committed. > > Do you think it's still worth trying to make it safe, or do you think > we should just remove exclusive mode completely? My preference would be to remove it completely, but I haven't gotten a lot of traction so far. >> Attaching the thread [1] that I started with a patch to remove exclusive >> backup for reference. > > Ah, good, some light reading. :) Sure, if you say so! Regards, -David ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-12-01 18:33 Bossart, Nathan <[email protected]> parent: David Steele <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Bossart, Nathan @ 2021-12-01 18:33 UTC (permalink / raw) To: David Steele <[email protected]>; Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 12/1/21, 8:27 AM, "David Steele" <[email protected]> wrote: > On 11/30/21 18:31, Bossart, Nathan wrote: >> Do you think it's still worth trying to make it safe, or do you think >> we should just remove exclusive mode completely? > > My preference would be to remove it completely, but I haven't gotten a > lot of traction so far. In this thread, I count 6 people who seem alright with removing it, and 2 who might be opposed, although I don't think anyone has explicitly stated they are against it. Nathan ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-12-02 00:30 Bossart, Nathan <[email protected]> parent: Bossart, Nathan <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Bossart, Nathan @ 2021-12-02 00:30 UTC (permalink / raw) To: David Steele <[email protected]>; Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 12/1/21, 10:37 AM, "Bossart, Nathan" <[email protected]> wrote: > On 12/1/21, 8:27 AM, "David Steele" <[email protected]> wrote: >> On 11/30/21 18:31, Bossart, Nathan wrote: >>> Do you think it's still worth trying to make it safe, or do you think >>> we should just remove exclusive mode completely? >> >> My preference would be to remove it completely, but I haven't gotten a >> lot of traction so far. > > In this thread, I count 6 people who seem alright with removing it, > and 2 who might be opposed, although I don't think anyone has > explicitly stated they are against it. I hastily rebased the patch from 2018 and got it building and passing the tests. I'm sure it will need additional changes, but I'll wait for more feedback before I expend too much more effort on this. Nathan Attachments: [application/octet-stream] v1-0001-remove-exclusive-backup-mode.patch (58.7K, ../../[email protected]/2-v1-0001-remove-exclusive-backup-mode.patch) download | inline diff: From f132dd976e7c92866bb38fbd6665ae87a4600eda Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 1 Dec 2021 23:50:49 +0000 Subject: [PATCH v1 1/1] remove exclusive backup mode --- doc/src/sgml/backup.sgml | 154 +------ doc/src/sgml/func.sgml | 30 +- src/backend/access/transam/xlog.c | 466 ++------------------- src/backend/access/transam/xlogfuncs.c | 235 ++--------- src/backend/catalog/system_functions.sql | 14 +- src/backend/postmaster/postmaster.c | 46 +- src/backend/replication/basebackup.c | 6 +- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 4 + src/bin/pg_rewind/filemap.c | 6 +- src/include/access/xlog.h | 3 +- src/include/catalog/pg_proc.dat | 20 +- src/include/miscadmin.h | 4 - src/test/perl/PostgreSQL/Test/Cluster.pm | 56 +-- .../recovery/t/010_logical_decoding_timelines.pl | 4 +- 14 files changed, 113 insertions(+), 935 deletions(-) diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml index cba32b6eb3..9a8b9f84ea 100644 --- a/doc/src/sgml/backup.sgml +++ b/doc/src/sgml/backup.sgml @@ -837,16 +837,8 @@ test ! -f /mnt/server/archivedir/00000001000000A900000065 && cp pg_wal/0 sequence, and that the success of a step is verified before proceeding to the next step. </para> - <para> - Low level base backups can be made in a non-exclusive or an exclusive - way. The non-exclusive method is recommended and the exclusive one is - deprecated and will eventually be removed. - </para> - - <sect3 id="backup-lowlevel-base-backup-nonexclusive"> - <title>Making a Non-Exclusive Low-Level Backup</title> <para> - A non-exclusive low level backup is one that allows other + A low level backup allows other concurrent backups to be running (both those started using the same backup API and those started using <xref linkend="app-pgbasebackup"/>). @@ -864,7 +856,7 @@ test ! -f /mnt/server/archivedir/00000001000000A900000065 && cp pg_wal/0 rights to run pg_start_backup (superuser, or a user who has been granted EXECUTE on the function) and issue the command: <programlisting> -SELECT pg_start_backup('label', false, false); +SELECT pg_start_backup('label', false); </programlisting> where <literal>label</literal> is any string you want to use to uniquely identify this backup operation. The connection @@ -885,10 +877,6 @@ SELECT pg_start_backup('label', false, false); issue an immediate checkpoint using as much I/O as available. </para> - <para> - The third parameter being <literal>false</literal> tells - <function>pg_start_backup</function> to initiate a non-exclusive base backup. - </para> </listitem> <listitem> <para> @@ -906,7 +894,7 @@ SELECT pg_start_backup('label', false, false); <para> In the same connection as before, issue the command: <programlisting> -SELECT * FROM pg_stop_backup(false, true); +SELECT * FROM pg_stop_backup(true); </programlisting> This terminates backup mode. On a primary, it also performs an automatic switch to the next WAL segment. On a standby, it is not possible to @@ -965,142 +953,6 @@ SELECT * FROM pg_stop_backup(false, true); </listitem> </orderedlist> </para> - </sect3> - <sect3 id="backup-lowlevel-base-backup-exclusive"> - <title>Making an Exclusive Low-Level Backup</title> - - <note> - <para> - The exclusive backup method is deprecated and should be avoided. - Prior to <productname>PostgreSQL</productname> 9.6, this was the only - low-level method available, but it is now recommended that all users - upgrade their scripts to use non-exclusive backups. - </para> - </note> - - <para> - The process for an exclusive backup is mostly the same as for a - non-exclusive one, but it differs in a few key steps. This type of - backup can only be taken on a primary and does not allow concurrent - backups. Moreover, because it creates a backup label file, as - described below, it can block automatic restart of the primary server - after a crash. On the other hand, the erroneous removal of this - file from a backup or standby is a common mistake, which can result - in serious data corruption. If it is necessary to use this method, - the following steps may be used. - </para> - <para> - <orderedlist> - <listitem> - <para> - Ensure that WAL archiving is enabled and working. - </para> - </listitem> - <listitem> - <para> - Connect to the server (it does not matter which database) as a user with - rights to run pg_start_backup (superuser, or a user who has been granted - EXECUTE on the function) and issue the command: -<programlisting> -SELECT pg_start_backup('label'); -</programlisting> - where <literal>label</literal> is any string you want to use to uniquely - identify this backup operation. - <function>pg_start_backup</function> creates a <firstterm>backup label</firstterm> file, - called <filename>backup_label</filename>, in the cluster directory with - information about your backup, including the start time and label string. - The function also creates a <firstterm>tablespace map</firstterm> file, - called <filename>tablespace_map</filename>, in the cluster directory with - information about tablespace symbolic links in <filename>pg_tblspc/</filename> if - one or more such link is present. Both files are critical to the - integrity of the backup, should you need to restore from it. - </para> - - <para> - By default, <function>pg_start_backup</function> can take a long time to finish. - This is because it performs a checkpoint, and the I/O - required for the checkpoint will be spread out over a significant - period of time, by default half your inter-checkpoint interval - (see the configuration parameter - <xref linkend="guc-checkpoint-completion-target"/>). This is - usually what you want, because it minimizes the impact on query - processing. If you want to start the backup as soon as - possible, use: -<programlisting> -SELECT pg_start_backup('label', true); -</programlisting> - This forces the checkpoint to be done as quickly as possible. - </para> - </listitem> - <listitem> - <para> - Perform the backup, using any convenient file-system-backup tool - such as <application>tar</application> or <application>cpio</application> (not - <application>pg_dump</application> or - <application>pg_dumpall</application>). It is neither - necessary nor desirable to stop normal operation of the database - while you do this. See - <xref linkend="backup-lowlevel-base-backup-data"/> for things to - consider during this backup. - </para> - <para> - As noted above, if the server crashes during the backup it may not be - possible to restart until the <filename>backup_label</filename> file has - been manually deleted from the <envar>PGDATA</envar> directory. Note - that it is very important to never remove the - <filename>backup_label</filename> file when restoring a backup, because - this will result in corruption. Confusion about when it is appropriate - to remove this file is a common cause of data corruption when using this - method; be very certain that you remove the file only on an existing - primary and never when building a standby or restoring a backup, even if - you are building a standby that will subsequently be promoted to a new - primary. - </para> - </listitem> - <listitem> - <para> - Again connect to the database as a user with rights to run - pg_stop_backup (superuser, or a user who has been granted EXECUTE on - the function), and issue the command: -<programlisting> -SELECT pg_stop_backup(); -</programlisting> - This function terminates backup mode and - performs an automatic switch to the next WAL segment. The reason for the - switch is to arrange for the last WAL segment written during the backup - interval to be ready to archive. - </para> - </listitem> - <listitem> - <para> - Once the WAL segment files active during the backup are archived, you are - done. The file identified by <function>pg_stop_backup</function>'s result is - the last segment that is required to form a complete set of backup files. - If <varname>archive_mode</varname> is enabled, - <function>pg_stop_backup</function> does not return until the last segment has - been archived. - Archiving of these files happens automatically since you have - already configured <varname>archive_command</varname>. In most cases this - happens quickly, but you are advised to monitor your archive - system to ensure there are no delays. - If the archive process has fallen behind - because of failures of the archive command, it will keep retrying - until the archive succeeds and the backup is complete. - </para> - - <para> - When using exclusive backup mode, it is absolutely imperative to ensure - that <function>pg_stop_backup</function> completes successfully at the - end of the backup. Even if the backup itself fails, for example due to - lack of disk space, failure to call <function>pg_stop_backup</function> - will leave the server in backup mode indefinitely, causing future backups - to fail and increasing the risk of a restart failure during the time that - <filename>backup_label</filename> exists. - </para> - </listitem> - </orderedlist> - </para> - </sect3> <sect3 id="backup-lowlevel-base-backup-data"> <title>Backing Up the Data Directory</title> <para> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 0a725a6711..28db66bb77 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -25580,8 +25580,7 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560 <function>pg_start_backup</function> ( <parameter>label</parameter> <type>text</type> <optional>, <parameter>fast</parameter> <type>boolean</type> - <optional>, <parameter>exclusive</parameter> <type>boolean</type> - </optional></optional> ) + </optional> ) <returnvalue>pg_lsn</returnvalue> </para> <para> @@ -25593,20 +25592,6 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560 it specifies executing <function>pg_start_backup</function> as quickly as possible. This forces an immediate checkpoint which will cause a spike in I/O operations, slowing any concurrently executing queries. - The optional third parameter specifies whether to perform an exclusive - or non-exclusive backup (default is exclusive). - </para> - <para> - When used in exclusive mode, this function writes a backup label file - (<filename>backup_label</filename>) and, if there are any links in - the <filename>pg_tblspc/</filename> directory, a tablespace map file - (<filename>tablespace_map</filename>) into the database cluster's data - directory, then performs a checkpoint, and then returns the backup's - starting write-ahead log location. (The user can ignore this - result value, but it is provided in case it is useful.) When used in - non-exclusive mode, the contents of these files are instead returned - by the <function>pg_stop_backup</function> function, and should be - copied to the backup area by the user. </para> <para> This function is restricted to superusers by default, but other users @@ -25620,7 +25605,6 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560 <primary>pg_stop_backup</primary> </indexterm> <function>pg_stop_backup</function> ( - <parameter>exclusive</parameter> <type>boolean</type> <optional>, <parameter>wait_for_archive</parameter> <type>boolean</type> </optional> ) <returnvalue>setof record</returnvalue> @@ -25629,13 +25613,8 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560 <parameter>spcmapfile</parameter> <type>text</type> ) </para> <para> - Finishes performing an exclusive or non-exclusive on-line backup. - The <parameter>exclusive</parameter> parameter must match the - previous <function>pg_start_backup</function> call. - In an exclusive backup, <function>pg_stop_backup</function> removes - the backup label file and, if it exists, the tablespace map file - created by <function>pg_start_backup</function>. In a non-exclusive - backup, the desired contents of these files are returned as part of + Finishes performing an on-line backup. The desired contents of the + backup label file and the tablespace map file are returned as part of the result of the function, and should be written to files in the backup area (not in the data directory). </para> @@ -25668,8 +25647,7 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560 The result of the function is a single record. The <parameter>lsn</parameter> column holds the backup's ending write-ahead log location (which again can be ignored). The second and - third columns are <literal>NULL</literal> when ending an exclusive - backup; after a non-exclusive backup they hold the desired contents of + third columns hold the desired contents of the label and tablespace map files. </para> <para> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index d894af310a..d4ca313d8a 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -482,29 +482,6 @@ typedef union WALInsertLockPadded char pad[PG_CACHE_LINE_SIZE]; } WALInsertLockPadded; -/* - * State of an exclusive backup, necessary to control concurrent activities - * across sessions when working on exclusive backups. - * - * EXCLUSIVE_BACKUP_NONE means that there is no exclusive backup actually - * running, to be more precise pg_start_backup() is not being executed for - * an exclusive backup and there is no exclusive backup in progress. - * EXCLUSIVE_BACKUP_STARTING means that pg_start_backup() is starting an - * exclusive backup. - * EXCLUSIVE_BACKUP_IN_PROGRESS means that pg_start_backup() has finished - * running and an exclusive backup is in progress. pg_stop_backup() is - * needed to finish it. - * EXCLUSIVE_BACKUP_STOPPING means that pg_stop_backup() is stopping an - * exclusive backup. - */ -typedef enum ExclusiveBackupState -{ - EXCLUSIVE_BACKUP_NONE = 0, - EXCLUSIVE_BACKUP_STARTING, - EXCLUSIVE_BACKUP_IN_PROGRESS, - EXCLUSIVE_BACKUP_STOPPING -} ExclusiveBackupState; - /* * Session status of running backup, used for sanity checks in SQL-callable * functions to start and stop backups. @@ -553,15 +530,12 @@ typedef struct XLogCtlInsert bool fullPageWrites; /* - * exclusiveBackupState indicates the state of an exclusive backup (see - * comments of ExclusiveBackupState for more details). nonExclusiveBackups - * is a counter indicating the number of streaming base 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. + * 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; + int runningBackups; XLogRecPtr lastBackupStart; /* @@ -958,7 +932,6 @@ static void xlog_outrec(StringInfo buf, XLogReaderState *record); static void xlog_block_info(StringInfo buf, XLogReaderState *record); static void xlog_outdesc(StringInfo buf, XLogReaderState *record); static void pg_start_backup_callback(int code, Datum arg); -static void pg_stop_backup_callback(int code, Datum arg); static bool read_backup_label(XLogRecPtr *checkPointLoc, TimeLineID *backupLabelTLI, bool *backupEndRequired, bool *backupFromStandby); @@ -9801,7 +9774,7 @@ CreateRestartPoint(int flags) * Ensure minRecoveryPoint is past the checkpoint record. Normally, * this will have happened already while writing out dirty buffers, * but not necessarily - e.g. because no buffers were dirtied. We do - * this because a non-exclusive base backup uses minRecoveryPoint to + * this because a backup performed in recovery uses minRecoveryPoint to * determine which WAL files must be included in the backup, and the * file (or files) containing the checkpoint record must be included, * at a minimum. Note that for an ordinary restart of recovery there's @@ -10968,32 +10941,14 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli) } /* - * do_pg_start_backup - * - * Utility function called at the start of an online backup. It creates the - * necessary starting checkpoint and constructs the backup label file. + * do_pg_start_backup is the workhorse of the user-visible pg_start_backup() + * function. It creates the necessary starting checkpoint and constructs the + * backup label and tablespace map. * - * There are two kind of backups: exclusive and non-exclusive. An exclusive - * backup is started with pg_start_backup(), and there can be only one active - * at a time. The backup and tablespace map files of an exclusive backup are - * written to $PGDATA/backup_label and $PGDATA/tablespace_map, and they are - * removed by pg_stop_backup(). - * - * A non-exclusive backup is used for the streaming base backups (see - * src/backend/replication/basebackup.c). The difference to exclusive backups - * is that the backup label and tablespace map files are not written to disk. - * Instead, their would-be contents are returned in *labelfile and *tblspcmapfile, - * and the caller is responsible for including them in the backup archive as - * 'backup_label' and 'tablespace_map'. There can be many non-exclusive backups - * active at the same time, and they don't conflict with an exclusive backup - * either. - * - * labelfile and tblspcmapfile must be passed as NULL when starting an - * exclusive backup, and as initially-empty StringInfos for a non-exclusive - * backup. - * - * If "tablespaces" isn't NULL, it receives a list of tablespaceinfo structs - * describing the cluster's tablespaces. + * The backup label and tablespace map contents are returned in *labelfile and + * *tblspcmapfile, and the caller is responsible for including them in the + * backup archive as 'backup_label' and 'tablespace_map'. There can be many + * backups active at the same time. * * tblspcmapfile is required mainly for tar format in windows as native windows * utilities are not able to create symlinks while extracting files from tar. @@ -11002,7 +10957,7 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli) * Returns the minimum WAL location that must be present to restore from this * backup, and the corresponding timeline ID in *starttli_p. * - * Every successfully started non-exclusive backup must be stopped by calling + * Every successfully started backup must be stopped by calling * do_pg_stop_backup() or do_pg_abort_backup(). * * It is the responsibility of the caller of this function to verify the @@ -11013,7 +10968,6 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p, StringInfo labelfile, List **tablespaces, StringInfo tblspcmapfile) { - bool exclusive = (labelfile == NULL); bool backup_started_in_recovery = false; XLogRecPtr checkpointloc; XLogRecPtr startpoint; @@ -11022,20 +10976,9 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p, char strfbuf[128]; char xlogfilename[MAXFNAMELEN]; XLogSegNo _logSegNo; - struct stat stat_buf; - FILE *fp; backup_started_in_recovery = RecoveryInProgress(); - /* - * Currently only non-exclusive backup can be taken during recovery. - */ - if (backup_started_in_recovery && exclusive) - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("recovery is in progress"), - errhint("WAL control functions cannot be executed during recovery."))); - /* * During recovery, we don't need to check WAL level. Because, if WAL * level is not sufficient, it's impossible to get here during recovery. @@ -11074,30 +11017,12 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p, * XLogInsertRecord(). */ WALInsertLockAcquireExclusive(); - if (exclusive) - { - /* - * At first, mark that we're now starting an exclusive backup, to - * ensure that there are no other sessions currently running - * pg_start_backup() or pg_stop_backup(). - */ - if (XLogCtl->Insert.exclusiveBackupState != EXCLUSIVE_BACKUP_NONE) - { - WALInsertLockRelease(); - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("a backup is already in progress"), - errhint("Run pg_stop_backup() and try again."))); - } - XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_STARTING; - } - else - XLogCtl->Insert.nonExclusiveBackups++; + XLogCtl->Insert.runningBackups++; XLogCtl->Insert.forcePageWrites = true; WALInsertLockRelease(); /* Ensure we release forcePageWrites if fail below */ - PG_ENSURE_ERROR_CLEANUP(pg_start_backup_callback, (Datum) BoolGetDatum(exclusive)); + PG_ENSURE_ERROR_CLEANUP(pg_start_backup_callback, (Datum) 0); { bool gotUniqueStartpoint = false; DIR *tblspcdir; @@ -11332,122 +11257,19 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p, LSN_FORMAT_ARGS(startpoint), xlogfilename); appendStringInfo(labelfile, "CHECKPOINT LOCATION: %X/%X\n", LSN_FORMAT_ARGS(checkpointloc)); - appendStringInfo(labelfile, "BACKUP METHOD: %s\n", - exclusive ? "pg_start_backup" : "streamed"); + appendStringInfo(labelfile, "BACKUP METHOD: streamed\n"); appendStringInfo(labelfile, "BACKUP FROM: %s\n", backup_started_in_recovery ? "standby" : "primary"); appendStringInfo(labelfile, "START TIME: %s\n", strfbuf); appendStringInfo(labelfile, "LABEL: %s\n", backupidstr); appendStringInfo(labelfile, "START TIMELINE: %u\n", starttli); - - /* - * Okay, write the file, or return its contents to caller. - */ - if (exclusive) - { - /* - * Check for existing backup label --- implies a backup is already - * running. (XXX given that we checked exclusiveBackupState - * above, maybe it would be OK to just unlink any such label - * file?) - */ - if (stat(BACKUP_LABEL_FILE, &stat_buf) != 0) - { - if (errno != ENOENT) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not stat file \"%s\": %m", - BACKUP_LABEL_FILE))); - } - else - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("a backup is already in progress"), - errhint("If you're sure there is no backup in progress, remove file \"%s\" and try again.", - BACKUP_LABEL_FILE))); - - fp = AllocateFile(BACKUP_LABEL_FILE, "w"); - - if (!fp) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not create file \"%s\": %m", - BACKUP_LABEL_FILE))); - if (fwrite(labelfile->data, labelfile->len, 1, fp) != 1 || - fflush(fp) != 0 || - pg_fsync(fileno(fp)) != 0 || - ferror(fp) || - FreeFile(fp)) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not write file \"%s\": %m", - BACKUP_LABEL_FILE))); - /* Allocated locally for exclusive backups, so free separately */ - pfree(labelfile->data); - pfree(labelfile); - - /* Write backup tablespace_map file. */ - if (tblspcmapfile->len > 0) - { - if (stat(TABLESPACE_MAP, &stat_buf) != 0) - { - if (errno != ENOENT) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not stat file \"%s\": %m", - TABLESPACE_MAP))); - } - else - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("a backup is already in progress"), - errhint("If you're sure there is no backup in progress, remove file \"%s\" and try again.", - TABLESPACE_MAP))); - - fp = AllocateFile(TABLESPACE_MAP, "w"); - - if (!fp) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not create file \"%s\": %m", - TABLESPACE_MAP))); - if (fwrite(tblspcmapfile->data, tblspcmapfile->len, 1, fp) != 1 || - fflush(fp) != 0 || - pg_fsync(fileno(fp)) != 0 || - ferror(fp) || - FreeFile(fp)) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not write file \"%s\": %m", - TABLESPACE_MAP))); - } - - /* Allocated locally for exclusive backups, so free separately */ - pfree(tblspcmapfile->data); - pfree(tblspcmapfile); - } } - PG_END_ENSURE_ERROR_CLEANUP(pg_start_backup_callback, (Datum) BoolGetDatum(exclusive)); + PG_END_ENSURE_ERROR_CLEANUP(pg_start_backup_callback, (Datum) 0); /* - * Mark that start phase has correctly finished for an exclusive backup. - * Session-level locks are updated as well to reflect that state. - * - * Note that CHECK_FOR_INTERRUPTS() must not occur while updating backup - * counters and session-level lock. Otherwise they can be updated - * inconsistently, and which might cause do_pg_abort_backup() to fail. + * Mark that the start phase has correctly finished for the backup. */ - if (exclusive) - { - WALInsertLockAcquireExclusive(); - XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_IN_PROGRESS; - - /* Set session-level lock */ - sessionBackupState = SESSION_BACKUP_EXCLUSIVE; - WALInsertLockRelease(); - } - else - sessionBackupState = SESSION_BACKUP_NON_EXCLUSIVE; + sessionBackupState = SESSION_BACKUP_RUNNING; /* * We're done. As a convenience, return the starting WAL location. @@ -11461,43 +11283,15 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p, static void pg_start_backup_callback(int code, Datum arg) { - bool exclusive = DatumGetBool(arg); - /* Update backup counters and forcePageWrites on failure */ WALInsertLockAcquireExclusive(); - if (exclusive) - { - Assert(XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_STARTING); - XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_NONE; - } - else - { - Assert(XLogCtl->Insert.nonExclusiveBackups > 0); - XLogCtl->Insert.nonExclusiveBackups--; - } - if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_NONE && - XLogCtl->Insert.nonExclusiveBackups == 0) - { - XLogCtl->Insert.forcePageWrites = false; - } - WALInsertLockRelease(); -} - -/* - * Error cleanup callback for pg_stop_backup - */ -static void -pg_stop_backup_callback(int code, Datum arg) -{ - bool exclusive = DatumGetBool(arg); + Assert(XLogCtl->Insert.runningBackups > 0); + XLogCtl->Insert.runningBackups--; - /* Update backup status on failure */ - WALInsertLockAcquireExclusive(); - if (exclusive) + if (XLogCtl->Insert.runningBackups == 0) { - Assert(XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_STOPPING); - XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_IN_PROGRESS; + XLogCtl->Insert.forcePageWrites = false; } WALInsertLockRelease(); } @@ -11517,9 +11311,6 @@ get_backup_status(void) * Utility function called at the end of an online backup. It cleans up the * backup state and can optionally wait for WAL segments to be archived. * - * If labelfile is NULL, this stops an exclusive backup. Otherwise this stops - * the non-exclusive backup specified by 'labelfile'. - * * Returns the last WAL location that must be present to restore from this * backup, and the corresponding timeline ID in *stoptli_p. * @@ -11529,7 +11320,6 @@ get_backup_status(void) XLogRecPtr do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) { - bool exclusive = (labelfile == NULL); bool backup_started_in_recovery = false; XLogRecPtr startpoint; XLogRecPtr stoppoint; @@ -11543,7 +11333,6 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) char histfilename[MAXFNAMELEN]; char backupfrom[20]; XLogSegNo _logSegNo; - FILE *lfp; FILE *fp; char ch; int seconds_before_warning; @@ -11556,15 +11345,6 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) backup_started_in_recovery = RecoveryInProgress(); - /* - * Currently only non-exclusive backup can be taken during recovery. - */ - if (backup_started_in_recovery && exclusive) - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("recovery is in progress"), - errhint("WAL control functions cannot be executed during recovery."))); - /* * During recovery, we don't need to check WAL level. Because, if WAL * level is not sufficient, it's impossible to get here during recovery. @@ -11575,106 +11355,23 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) errmsg("WAL level not sufficient for making an online backup"), errhint("wal_level must be set to \"replica\" or \"logical\" at server start."))); - if (exclusive) - { - /* - * At first, mark that we're now stopping an exclusive backup, to - * ensure that there are no other sessions currently running - * pg_start_backup() or pg_stop_backup(). - */ - WALInsertLockAcquireExclusive(); - if (XLogCtl->Insert.exclusiveBackupState != EXCLUSIVE_BACKUP_IN_PROGRESS) - { - WALInsertLockRelease(); - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("exclusive backup not in progress"))); - } - XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_STOPPING; - WALInsertLockRelease(); - - /* - * Remove backup_label. In case of failure, the state for an exclusive - * backup is switched back to in-progress. - */ - PG_ENSURE_ERROR_CLEANUP(pg_stop_backup_callback, (Datum) BoolGetDatum(exclusive)); - { - /* - * Read the existing label file into memory. - */ - struct stat statbuf; - int r; - - if (stat(BACKUP_LABEL_FILE, &statbuf)) - { - /* should not happen per the upper checks */ - if (errno != ENOENT) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not stat file \"%s\": %m", - BACKUP_LABEL_FILE))); - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("a backup is not in progress"))); - } - - lfp = AllocateFile(BACKUP_LABEL_FILE, "r"); - if (!lfp) - { - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not read file \"%s\": %m", - BACKUP_LABEL_FILE))); - } - labelfile = palloc(statbuf.st_size + 1); - r = fread(labelfile, statbuf.st_size, 1, lfp); - labelfile[statbuf.st_size] = '\0'; - - /* - * Close and remove the backup label file - */ - if (r != 1 || ferror(lfp) || FreeFile(lfp)) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not read file \"%s\": %m", - BACKUP_LABEL_FILE))); - durable_unlink(BACKUP_LABEL_FILE, ERROR); - - /* - * Remove tablespace_map file if present, it is created only if - * there are tablespaces. - */ - durable_unlink(TABLESPACE_MAP, DEBUG1); - } - PG_END_ENSURE_ERROR_CLEANUP(pg_stop_backup_callback, (Datum) BoolGetDatum(exclusive)); - } - /* - * OK to update backup counters, forcePageWrites and session-level lock. + * OK to update backup counters and forcePageWrites. * * Note that CHECK_FOR_INTERRUPTS() must not occur while updating them. * Otherwise they can be updated inconsistently, and which might cause * do_pg_abort_backup() to fail. */ WALInsertLockAcquireExclusive(); - if (exclusive) - { - XLogCtl->Insert.exclusiveBackupState = EXCLUSIVE_BACKUP_NONE; - } - else - { - /* - * The user-visible pg_start/stop_backup() functions that operate on - * exclusive backups can be called at any time, but for non-exclusive - * backups, it is expected that each do_pg_start_backup() call is - * matched by exactly one do_pg_stop_backup() call. - */ - Assert(XLogCtl->Insert.nonExclusiveBackups > 0); - XLogCtl->Insert.nonExclusiveBackups--; - } - if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_NONE && - XLogCtl->Insert.nonExclusiveBackups == 0) + /* + * It is expected that each do_pg_start_backup() call is matched by exactly + * one do_pg_stop_backup() call. + */ + Assert(XLogCtl->Insert.runningBackups > 0); + XLogCtl->Insert.runningBackups--; + + if (XLogCtl->Insert.runningBackups == 0) { XLogCtl->Insert.forcePageWrites = false; } @@ -11939,10 +11636,6 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p) * The caller can pass 'arg' as 'true' or 'false' to control whether a warning * is emitted. * - * NB: This is only for aborting a non-exclusive backup that doesn't write - * backup_label. A backup started with pg_start_backup() needs to be finished - * with pg_stop_backup(). - * * NB: This gets used as a before_shmem_exit handler, hence the odd-looking * signature. */ @@ -11952,18 +11645,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; } @@ -12308,87 +11999,6 @@ rm_redo_error_callback(void *arg) pfree(buf.data); } -/* - * BackupInProgress: check if online backup mode is active - * - * This is done by checking for existence of the "backup_label" file. - */ -bool -BackupInProgress(void) -{ - struct stat stat_buf; - - return (stat(BACKUP_LABEL_FILE, &stat_buf) == 0); -} - -/* - * CancelBackup: rename the "backup_label" and "tablespace_map" - * files to cancel backup mode - * - * If the "backup_label" file exists, it will be renamed to "backup_label.old". - * Similarly, if the "tablespace_map" file exists, it will be renamed to - * "tablespace_map.old". - * - * Note that this will render an online backup in progress - * useless. To correctly finish an online backup, pg_stop_backup must be - * called. - */ -void -CancelBackup(void) -{ - struct stat stat_buf; - - /* if the backup_label file is not there, return */ - if (stat(BACKUP_LABEL_FILE, &stat_buf) < 0) - return; - - /* remove leftover file from previously canceled backup if it exists */ - unlink(BACKUP_LABEL_OLD); - - if (durable_rename(BACKUP_LABEL_FILE, BACKUP_LABEL_OLD, DEBUG1) != 0) - { - ereport(WARNING, - (errcode_for_file_access(), - errmsg("online backup mode was not canceled"), - errdetail("File \"%s\" could not be renamed to \"%s\": %m.", - BACKUP_LABEL_FILE, BACKUP_LABEL_OLD))); - return; - } - - /* if the tablespace_map file is not there, return */ - if (stat(TABLESPACE_MAP, &stat_buf) < 0) - { - ereport(LOG, - (errmsg("online backup mode canceled"), - errdetail("File \"%s\" was renamed to \"%s\".", - BACKUP_LABEL_FILE, BACKUP_LABEL_OLD))); - return; - } - - /* remove leftover file from previously canceled backup if it exists */ - unlink(TABLESPACE_MAP_OLD); - - if (durable_rename(TABLESPACE_MAP, TABLESPACE_MAP_OLD, DEBUG1) == 0) - { - ereport(LOG, - (errmsg("online backup mode canceled"), - errdetail("Files \"%s\" and \"%s\" were renamed to " - "\"%s\" and \"%s\", respectively.", - BACKUP_LABEL_FILE, TABLESPACE_MAP, - BACKUP_LABEL_OLD, TABLESPACE_MAP_OLD))); - } - else - { - ereport(WARNING, - (errcode_for_file_access(), - errmsg("online backup mode canceled"), - errdetail("File \"%s\" was renamed to \"%s\", but " - "file \"%s\" could not be renamed to \"%s\": %m.", - BACKUP_LABEL_FILE, BACKUP_LABEL_OLD, - TABLESPACE_MAP, TABLESPACE_MAP_OLD))); - } -} - /* * Read the XLOG page containing RecPtr into readBuf (if not read already). * Returns number of bytes read, if the page is read successfully, or -1 diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index dd9a45c186..aab5dd88fa 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -39,7 +39,7 @@ #include "utils/tuplestore.h" /* - * Store label file and tablespace map during non-exclusive backups. + * Store label file and tablespace map during backups. */ static StringInfo label_file; static StringInfo tblspc_map_file; @@ -61,101 +61,40 @@ pg_start_backup(PG_FUNCTION_ARGS) { text *backupid = PG_GETARG_TEXT_PP(0); bool fast = PG_GETARG_BOOL(1); - bool exclusive = PG_GETARG_BOOL(2); char *backupidstr; XLogRecPtr startpoint; SessionBackupState status = get_backup_status(); + MemoryContext oldcontext; backupidstr = text_to_cstring(backupid); - if (status == SESSION_BACKUP_NON_EXCLUSIVE) + if (status == SESSION_BACKUP_RUNNING) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("a backup is already in progress in this session"))); - if (exclusive) - { - startpoint = do_pg_start_backup(backupidstr, fast, NULL, NULL, - NULL, NULL); - } - else - { - MemoryContext oldcontext; - - /* - * Label file and tablespace map file need to be long-lived, since - * they are read in pg_stop_backup. - */ - oldcontext = MemoryContextSwitchTo(TopMemoryContext); - label_file = makeStringInfo(); - tblspc_map_file = makeStringInfo(); - MemoryContextSwitchTo(oldcontext); + /* + * Label file and tablespace map file need to be long-lived, since + * they are read in pg_stop_backup. + */ + oldcontext = MemoryContextSwitchTo(TopMemoryContext); + label_file = makeStringInfo(); + tblspc_map_file = makeStringInfo(); + MemoryContextSwitchTo(oldcontext); - register_persistent_abort_backup_handler(); + register_persistent_abort_backup_handler(); - startpoint = do_pg_start_backup(backupidstr, fast, NULL, label_file, - NULL, tblspc_map_file); - } + startpoint = do_pg_start_backup(backupidstr, fast, NULL, label_file, + NULL, tblspc_map_file); PG_RETURN_LSN(startpoint); } -/* - * pg_stop_backup: finish taking an on-line backup dump - * - * We write an end-of-backup WAL record, and remove the backup label file - * created by pg_start_backup, creating a backup history file in pg_wal - * instead (whence it will immediately be archived). The backup history file - * contains the same info found in the label file, plus the backup-end time - * and WAL location. Before 9.0, the backup-end time was read from the backup - * history file at the beginning of archive recovery, but we now use the WAL - * record for that and the file is for informational and debug purposes only. - * - * Note: different from CancelBackup which just cancels online backup mode. - * - * Note: this version is only called to stop an exclusive backup. The function - * pg_stop_backup_v2 (overloaded as pg_stop_backup in SQL) is called to - * stop non-exclusive backups. - * - * Permission checking for this function is managed through the normal - * GRANT system. - */ -Datum -pg_stop_backup(PG_FUNCTION_ARGS) -{ - XLogRecPtr stoppoint; - SessionBackupState status = get_backup_status(); - - if (status == SESSION_BACKUP_NON_EXCLUSIVE) - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("non-exclusive backup in progress"), - errhint("Did you mean to use pg_stop_backup('f')?"))); - - /* - * Exclusive backups were typically started in a different connection, so - * don't try to verify that status of backup is set to - * SESSION_BACKUP_EXCLUSIVE in this function. Actual verification that an - * exclusive backup is in fact running is handled inside - * do_pg_stop_backup. - */ - stoppoint = do_pg_stop_backup(NULL, true, NULL); - - PG_RETURN_LSN(stoppoint); -} - /* - * pg_stop_backup_v2: finish taking exclusive or nonexclusive on-line backup. - * - * Works the same as pg_stop_backup, except for non-exclusive backups it returns - * the backup label and tablespace map files as text fields in as part of the - * resultset. + * pg_stop_backup: finish taking an on-line backup. * - * The first parameter (variable 'exclusive') allows the user to tell us if - * this is an exclusive or a non-exclusive backup. - * - * The second parameter (variable 'waitforarchive'), which is optional, + * The first parameter (variable 'waitforarchive'), which is optional, * allows the user to choose if they want to wait for the WAL to be archived * or if we should just return as soon as the WAL record is written. * @@ -163,7 +102,7 @@ pg_stop_backup(PG_FUNCTION_ARGS) * GRANT system. */ Datum -pg_stop_backup_v2(PG_FUNCTION_ARGS) +pg_stop_backup(PG_FUNCTION_ARGS) { ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; TupleDesc tupdesc; @@ -173,8 +112,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS) Datum values[3]; bool nulls[3]; - bool exclusive = PG_GETARG_BOOL(0); - bool waitforarchive = PG_GETARG_BOOL(1); + bool waitforarchive = PG_GETARG_BOOL(0); XLogRecPtr stoppoint; SessionBackupState status = get_backup_status(); @@ -205,51 +143,29 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS) MemSet(values, 0, sizeof(values)); MemSet(nulls, 0, sizeof(nulls)); - if (exclusive) - { - if (status == SESSION_BACKUP_NON_EXCLUSIVE) - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("non-exclusive backup in progress"), - errhint("Did you mean to use pg_stop_backup('f')?"))); - - /* - * Stop the exclusive backup, and since we're in an exclusive backup - * return NULL for both backup_label and tablespace_map. - */ - stoppoint = do_pg_stop_backup(NULL, waitforarchive, NULL); - - nulls[1] = true; - nulls[2] = true; - } - else - { - if (status != SESSION_BACKUP_NON_EXCLUSIVE) - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("non-exclusive backup is not in progress"), - errhint("Did you mean to use pg_stop_backup('t')?"))); + if (status != SESSION_BACKUP_RUNNING) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("backup is not in progress"), + errhint("Did you call pg_start_backup()?"))); - /* - * Stop the non-exclusive backup. Return a copy of the backup label - * and tablespace map so they can be written to disk by the caller. - */ - stoppoint = do_pg_stop_backup(label_file->data, waitforarchive, NULL); - - values[1] = CStringGetTextDatum(label_file->data); - values[2] = CStringGetTextDatum(tblspc_map_file->data); - - /* Free structures allocated in TopMemoryContext */ - pfree(label_file->data); - pfree(label_file); - label_file = NULL; - pfree(tblspc_map_file->data); - pfree(tblspc_map_file); - tblspc_map_file = NULL; - } + /* + * Stop the backup. Return a copy of the backup label and tablespace map so + * they can be written to disk by the caller. + */ + stoppoint = do_pg_stop_backup(label_file->data, waitforarchive, NULL); - /* Stoppoint is included on both exclusive and nonexclusive backups */ values[0] = LSNGetDatum(stoppoint); + values[1] = CStringGetTextDatum(label_file->data); + values[2] = CStringGetTextDatum(tblspc_map_file->data); + + /* Free structures allocated in TopMemoryContext */ + pfree(label_file->data); + pfree(label_file); + label_file = NULL; + pfree(tblspc_map_file->data); + pfree(tblspc_map_file); + tblspc_map_file = NULL; tuplestore_putvalues(tupstore, tupdesc, values, nulls); tuplestore_donestoring(tupstore); @@ -670,81 +586,6 @@ pg_wal_lsn_diff(PG_FUNCTION_ARGS) PG_RETURN_NUMERIC(result); } -/* - * Returns bool with current on-line backup mode, a global state. - */ -Datum -pg_is_in_backup(PG_FUNCTION_ARGS) -{ - PG_RETURN_BOOL(BackupInProgress()); -} - -/* - * Returns start time of an online exclusive backup. - * - * When there's no exclusive backup in progress, the function - * returns NULL. - */ -Datum -pg_backup_start_time(PG_FUNCTION_ARGS) -{ - Datum xtime; - FILE *lfp; - char fline[MAXPGPATH]; - char backup_start_time[30]; - - /* - * See if label file is present - */ - lfp = AllocateFile(BACKUP_LABEL_FILE, "r"); - if (lfp == NULL) - { - if (errno != ENOENT) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not read file \"%s\": %m", - BACKUP_LABEL_FILE))); - PG_RETURN_NULL(); - } - - /* - * Parse the file to find the START TIME line. - */ - backup_start_time[0] = '\0'; - while (fgets(fline, sizeof(fline), lfp) != NULL) - { - if (sscanf(fline, "START TIME: %25[^\n]\n", backup_start_time) == 1) - break; - } - - /* Check for a read error. */ - if (ferror(lfp)) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not read file \"%s\": %m", BACKUP_LABEL_FILE))); - - /* Close the backup label file. */ - if (FreeFile(lfp)) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not close file \"%s\": %m", BACKUP_LABEL_FILE))); - - if (strlen(backup_start_time) == 0) - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE))); - - /* - * Convert the time string read from file to TimestampTz form. - */ - xtime = DirectFunctionCall3(timestamptz_in, - CStringGetDatum(backup_start_time), - ObjectIdGetDatum(InvalidOid), - Int32GetDatum(-1)); - - PG_RETURN_DATUM(xtime); -} - /* * Promotes a standby server. * diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql index 3a4fa9091b..c8390cca06 100644 --- a/src/backend/catalog/system_functions.sql +++ b/src/backend/catalog/system_functions.sql @@ -377,14 +377,14 @@ BEGIN ATOMIC END; CREATE OR REPLACE FUNCTION - pg_start_backup(label text, fast boolean DEFAULT false, exclusive boolean DEFAULT true) + pg_start_backup(label text, fast boolean DEFAULT false) RETURNS pg_lsn STRICT VOLATILE LANGUAGE internal AS 'pg_start_backup' PARALLEL RESTRICTED; CREATE OR REPLACE FUNCTION pg_stop_backup ( - exclusive boolean, wait_for_archive boolean DEFAULT true, - OUT lsn pg_lsn, OUT labelfile text, OUT spcmapfile text) - RETURNS SETOF record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2' + wait_for_archive boolean DEFAULT true, OUT lsn pg_lsn, + OUT labelfile text, OUT spcmapfile text) + RETURNS SETOF record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup' PARALLEL RESTRICTED; CREATE OR REPLACE FUNCTION @@ -603,11 +603,9 @@ AS 'unicode_is_normalized'; -- available to superuser / cluster owner, if they choose. -- -REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean, boolean) FROM public; +REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean) FROM public; -REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public; - -REVOKE EXECUTE ON FUNCTION pg_stop_backup(boolean, boolean) FROM public; +REVOKE EXECUTE ON FUNCTION pg_stop_backup(boolean) FROM public; REVOKE EXECUTE ON FUNCTION pg_create_restore_point(text) FROM public; diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 328ecafa8c..fc84244c6a 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -347,7 +347,6 @@ static PMState pmState = PM_INIT; typedef enum { ALLOW_ALL_CONNS, /* normal not-shutting-down state */ - ALLOW_SUPERUSER_CONNS, /* only superusers can connect */ ALLOW_NO_CONNS /* no new connections allowed, period */ } ConnsAllowedState; @@ -2547,19 +2546,11 @@ canAcceptConnections(int backend_type) /* * "Smart shutdown" restrictions are applied only to normal connections, - * not to autovac workers or bgworkers. When only superusers can connect, - * we return CAC_SUPERUSER to indicate that superuserness must be checked - * later. Note that neither CAC_OK nor CAC_SUPERUSER can safely be - * returned until we have checked for too many children. + * not to autovac workers or bgworkers. */ if (connsAllowed != ALLOW_ALL_CONNS && backend_type == BACKEND_TYPE_NORMAL) - { - if (connsAllowed == ALLOW_SUPERUSER_CONNS) - result = CAC_SUPERUSER; /* allow superusers only */ - else - return CAC_SHUTDOWN; /* shutdown is pending */ - } + return CAC_SHUTDOWN; /* shutdown is pending */ /* * Don't start too many children. @@ -2878,16 +2869,11 @@ pmdie(SIGNAL_ARGS) #endif /* - * If we reached normal running, we have to wait for any online - * backup mode to end; otherwise go straight to waiting for client - * backends to exit. (The difference is that in the former state, - * we'll still let in new superuser clients, so that somebody can - * end the online backup mode.) If already in PM_STOP_BACKENDS or + * If we reached normal running, we go straight to waiting for + * client backends to exit. If already in PM_STOP_BACKENDS or * a later state, do not change it. */ - if (pmState == PM_RUN) - connsAllowed = ALLOW_SUPERUSER_CONNS; - else if (pmState == PM_HOT_STANDBY) + if (pmState == PM_RUN || pmState == PM_HOT_STANDBY) connsAllowed = ALLOW_NO_CONNS; else if (pmState == PM_STARTUP || pmState == PM_RECOVERY) { @@ -3843,16 +3829,6 @@ PostmasterStateMachine(void) /* If we're doing a smart shutdown, try to advance that state. */ if (pmState == PM_RUN || pmState == PM_HOT_STANDBY) { - if (connsAllowed == ALLOW_SUPERUSER_CONNS) - { - /* - * ALLOW_SUPERUSER_CONNS state ends as soon as online backup mode - * is not active. - */ - if (!BackupInProgress()) - connsAllowed = ALLOW_NO_CONNS; - } - if (connsAllowed == ALLOW_NO_CONNS) { /* @@ -4045,18 +4021,6 @@ PostmasterStateMachine(void) } else { - /* - * 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(); - /* * Normal exit from the postmaster is here. We don't need to log * anything here, since the UnlinkLockFiles proc_exit callback diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index ec0485705d..3d3b111c79 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -177,10 +177,8 @@ static const struct exclude_list_item excludeFiles[] = {RELCACHE_INIT_FILENAME, true}, /* - * If there's a backup_label or tablespace_map file, it belongs to a - * backup started by the user with pg_start_backup(). It is *not* correct - * for this backup. Our backup_label/tablespace_map is injected into the - * tar separately. + * backup_label and tablespace_map should not exist in in a running cluster + * capable of doing an online backup, but exclude then just in case. */ {BACKUP_LABEL_FILE, false}, {TABLESPACE_MAP, false}, diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index 89f45b77a3..f21d53086d 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -163,6 +163,10 @@ isnt(slurp_file("$tempdir/backup/backup_label"), 'DONOTCOPY', 'existing backup_label not copied'); rmtree("$tempdir/backup"); +# 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"); + $node->command_ok( [ 'pg_basebackup', '-D', diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c index 2618b4c957..37b669a3cb 100644 --- a/src/bin/pg_rewind/filemap.c +++ b/src/bin/pg_rewind/filemap.c @@ -140,9 +140,9 @@ static const struct exclude_list_item excludeFiles[] = {"pg_internal.init", true}, /* defined as RELCACHE_INIT_FILENAME */ /* - * If there's a backup_label or tablespace_map file, it belongs to a - * backup started by the user with pg_start_backup(). It is *not* correct - * for this backup. Our backup_label is written later on separately. + * If there is a backup_label or tablespace_map file, it indicates that + * a recovery failed and this cluster probably can't be rewound, but + * exclude them anyway if they are found. */ {"backup_label", false}, /* defined as BACKUP_LABEL_FILE */ {"tablespace_map", false}, /* defined as TABLESPACE_MAP */ diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 898df2ee03..f9f34f9792 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -343,8 +343,7 @@ extern void assign_checkpoint_completion_target(double newval, void *extra); typedef enum SessionBackupState { SESSION_BACKUP_NONE, - SESSION_BACKUP_EXCLUSIVE, - SESSION_BACKUP_NON_EXCLUSIVE + SESSION_BACKUP_RUNNING, } SessionBackupState; extern XLogRecPtr do_pg_start_backup(const char *backupidstr, bool fast, diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 79d787cd26..55b1f741f5 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -6282,25 +6282,15 @@ prosrc => 'pg_terminate_backend' }, { oid => '2172', descr => 'prepare for taking an online backup', proname => 'pg_start_backup', provolatile => 'v', proparallel => 'r', - prorettype => 'pg_lsn', proargtypes => 'text bool bool', + prorettype => 'pg_lsn', proargtypes => 'text bool', prosrc => 'pg_start_backup' }, -{ oid => '2173', descr => 'finish taking an online backup', - proname => 'pg_stop_backup', provolatile => 'v', proparallel => 'r', - prorettype => 'pg_lsn', proargtypes => '', prosrc => 'pg_stop_backup' }, { oid => '2739', descr => 'finish taking an online backup', proname => 'pg_stop_backup', prorows => '1', proretset => 't', provolatile => 'v', proparallel => 'r', prorettype => 'record', - proargtypes => 'bool bool', proallargtypes => '{bool,bool,pg_lsn,text,text}', - proargmodes => '{i,i,o,o,o}', - proargnames => '{exclusive,wait_for_archive,lsn,labelfile,spcmapfile}', - prosrc => 'pg_stop_backup_v2' }, -{ oid => '3813', descr => 'true if server is in online backup', - proname => 'pg_is_in_backup', provolatile => 'v', prorettype => 'bool', - proargtypes => '', prosrc => 'pg_is_in_backup' }, -{ oid => '3814', descr => 'start time of an online backup', - proname => 'pg_backup_start_time', provolatile => 's', - prorettype => 'timestamptz', proargtypes => '', - prosrc => 'pg_backup_start_time' }, + proargtypes => 'bool', proallargtypes => '{bool,pg_lsn,text,text}', + proargmodes => '{i,o,o,o}', + proargnames => '{wait_for_archive,lsn,labelfile,spcmapfile}', + prosrc => 'pg_stop_backup' }, { oid => '3436', descr => 'promote standby server', proname => 'pg_promote', provolatile => 'v', prorettype => 'bool', proargtypes => 'bool int4', proargnames => '{wait,wait_seconds}', diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 90a3016065..2c6454a51e 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -480,10 +480,6 @@ extern void process_session_preload_libraries(void); extern void pg_bindtextdomain(const char *domain); extern bool has_rolreplication(Oid roleid); -/* in access/transam/xlog.c */ -extern bool BackupInProgress(void); -extern void CancelBackup(void); - /* in executor/nodeHash.c */ extern size_t get_hash_memory_limit(void); diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm index 9467a199c8..55dc60ba2b 100644 --- a/src/test/perl/PostgreSQL/Test/Cluster.pm +++ b/src/test/perl/PostgreSQL/Test/Cluster.pm @@ -598,25 +598,6 @@ sub backup return; } -=item $node->backup_fs_hot(backup_name) - -Create a backup with a filesystem level copy in subdirectory B<backup_name> of -B<< $node->backup_dir >>, including WAL. - -Archiving must be enabled, as B<pg_start_backup()> and B<pg_stop_backup()> are -used. This is not checked or enforced. - -The backup name is passed as the backup label to B<pg_start_backup()>. - -=cut - -sub backup_fs_hot -{ - my ($self, $backup_name) = @_; - $self->_backup_fs($backup_name, 1); - return; -} - =item $node->backup_fs_cold(backup_name) Create a backup with a filesystem level copy in subdirectory B<backup_name> of @@ -630,53 +611,18 @@ Use B<backup> or B<backup_fs_hot> if you want to back up a running server. sub backup_fs_cold { my ($self, $backup_name) = @_; - $self->_backup_fs($backup_name, 0); - return; -} - - -# Common sub of backup_fs_hot and backup_fs_cold -sub _backup_fs -{ - my ($self, $backup_name, $hot) = @_; - my $backup_path = $self->backup_dir . '/' . $backup_name; - my $port = $self->port; - my $name = $self->name; - - print "# Taking filesystem backup $backup_name from node \"$name\"\n"; - - if ($hot) - { - my $stdout = $self->safe_psql('postgres', - "SELECT * FROM pg_start_backup('$backup_name');"); - print "# pg_start_backup: $stdout\n"; - } PostgreSQL::Test::RecursiveCopy::copypath( $self->data_dir, - $backup_path, + $self->backup_dir . '/' . $backup_name, filterfn => sub { my $src = shift; return ($src ne 'log' and $src ne 'postmaster.pid'); }); - if ($hot) - { - - # We ignore pg_stop_backup's return value. We also assume archiving - # is enabled; otherwise the caller will have to copy the remaining - # segments. - my $stdout = - $self->safe_psql('postgres', 'SELECT * FROM pg_stop_backup();'); - print "# pg_stop_backup: $stdout\n"; - } - - print "# Backup finished\n"; return; } - - =pod =item $node->init_from_backup(root_node, backup_name) diff --git a/src/test/recovery/t/010_logical_decoding_timelines.pl b/src/test/recovery/t/010_logical_decoding_timelines.pl index 68d94ac91c..2e7b72593f 100644 --- a/src/test/recovery/t/010_logical_decoding_timelines.pl +++ b/src/test/recovery/t/010_logical_decoding_timelines.pl @@ -69,7 +69,9 @@ $node_primary->safe_psql('dropme', $node_primary->safe_psql('postgres', 'CHECKPOINT;'); my $backup_name = 'b1'; -$node_primary->backup_fs_hot($backup_name); +$node_primary->stop(); +$node_primary->backup_fs_cold($backup_name); +$node_primary->start(); $node_primary->safe_psql('postgres', q[SELECT pg_create_physical_replication_slot('phys_slot');]); -- 2.16.6 ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-12-02 16:00 Andrew Dunstan <[email protected]> parent: Bossart, Nathan <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Andrew Dunstan @ 2021-12-02 16:00 UTC (permalink / raw) To: Bossart, Nathan <[email protected]>; David Steele <[email protected]>; Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 12/1/21 19:30, Bossart, Nathan wrote: > On 12/1/21, 10:37 AM, "Bossart, Nathan" <[email protected]> wrote: >> On 12/1/21, 8:27 AM, "David Steele" <[email protected]> wrote: >>> On 11/30/21 18:31, Bossart, Nathan wrote: >>>> Do you think it's still worth trying to make it safe, or do you think >>>> we should just remove exclusive mode completely? >>> My preference would be to remove it completely, but I haven't gotten a >>> lot of traction so far. >> In this thread, I count 6 people who seem alright with removing it, >> and 2 who might be opposed, although I don't think anyone has >> explicitly stated they are against it. > I hastily rebased the patch from 2018 and got it building and passing > the tests. I'm sure it will need additional changes, but I'll wait > for more feedback before I expend too much more effort on this. > Should we really be getting rid of PostgreSQL::Test::Cluster::backup_fs_hot() ? cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-12-02 17:38 David Steele <[email protected]> parent: Andrew Dunstan <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: David Steele @ 2021-12-02 17:38 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 12/2/21 11:00, Andrew Dunstan wrote: > > On 12/1/21 19:30, Bossart, Nathan wrote: >> On 12/1/21, 10:37 AM, "Bossart, Nathan" <[email protected]> wrote: >>> On 12/1/21, 8:27 AM, "David Steele" <[email protected]> wrote: >>>> On 11/30/21 18:31, Bossart, Nathan wrote: >>>>> Do you think it's still worth trying to make it safe, or do you think >>>>> we should just remove exclusive mode completely? >>>> My preference would be to remove it completely, but I haven't gotten a >>>> lot of traction so far. >>> In this thread, I count 6 people who seem alright with removing it, >>> and 2 who might be opposed, although I don't think anyone has >>> explicitly stated they are against it. >> I hastily rebased the patch from 2018 and got it building and passing >> the tests. I'm sure it will need additional changes, but I'll wait >> for more feedback before I expend too much more effort on this. >> > > Should we really be getting rid of > PostgreSQL::Test::Cluster::backup_fs_hot() ? Agreed, it would be better to update backup_fs_hot() to use exclusive mode and save out backup_label instead. Regards, -David ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-12-02 17:49 David Steele <[email protected]> parent: David Steele <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: David Steele @ 2021-12-02 17:49 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 12/2/21 12:38, David Steele wrote: > On 12/2/21 11:00, Andrew Dunstan wrote: >> >> Should we really be getting rid of >> PostgreSQL::Test::Cluster::backup_fs_hot() ? > > Agreed, it would be better to update backup_fs_hot() to use exclusive > mode and save out backup_label instead. Oops, of course I meant non-exclusive mode. Regards, -David ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2021-12-02 21:31 Bossart, Nathan <[email protected]> parent: David Steele <[email protected]> 0 siblings, 0 replies; 28+ messages in thread From: Bossart, Nathan @ 2021-12-02 21:31 UTC (permalink / raw) To: David Steele <[email protected]>; Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers; SATYANARAYANA NARLAPURAM <[email protected]> On 12/2/21, 9:50 AM, "David Steele" <[email protected]> wrote: > On 12/2/21 12:38, David Steele wrote: >> On 12/2/21 11:00, Andrew Dunstan wrote: >>> >>> Should we really be getting rid of >>> PostgreSQL::Test::Cluster::backup_fs_hot() ? >> >> Agreed, it would be better to update backup_fs_hot() to use exclusive >> mode and save out backup_label instead. > > Oops, of course I meant non-exclusive mode. +1. I'll fix that in the next revision. Nathan ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2022-03-28 20:30 Stephen Frost <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Stephen Frost @ 2022-03-28 20:30 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Chapman Flack <[email protected]>; Magnus Hagander <[email protected]>; David Steele <[email protected]>; [email protected] Greetings, * Nathan Bossart ([email protected]) wrote: > On Thu, Mar 10, 2022 at 07:13:14PM -0500, Chapman Flack wrote: > > Looks like this change to an example in func.sgml is not quite right: > > > > -postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); > > +postgres=# SELECT * FROM pg_walfile_name_offset(pg_backup_stop()); > > > > pg_backup_stop returns a record now, not just lsn. So this works for me: > > > > +postgres=# SELECT * FROM pg_walfile_name_offset((pg_backup_stop()).lsn); > > Ah, good catch. I made this change in v7. I considered doing something > like this > > SELECT w.* FROM pg_backup_stop() b, pg_walfile_name_offset(b.lsn) w; > > but I think your suggestion is simpler. I tend to agree. More below. > Subject: [PATCH v7 1/1] remove exclusive backup mode > diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml > index 0d69851bb1..c8b914c1aa 100644 > --- a/doc/src/sgml/backup.sgml > +++ b/doc/src/sgml/backup.sgml > @@ -881,19 +873,19 @@ test ! -f /mnt/server/archivedir/00000001000000A900000065 && cp pg_wal/0 > <listitem> > <para> > Connect to the server (it does not matter which database) as a user with > - rights to run pg_start_backup (superuser, or a user who has been granted > + rights to run pg_backup_start (superuser, or a user who has been granted > EXECUTE on the function) and issue the command: > <programlisting> > -SELECT pg_start_backup('label', false, false); > +SELECT pg_backup_start(label => 'label', fast => false); > </programlisting> > where <literal>label</literal> is any string you want to use to uniquely > identify this backup operation. The connection > - calling <function>pg_start_backup</function> must be maintained until the end of > + calling <function>pg_backup_start</function> must be maintained until the end of > the backup, or the backup will be automatically aborted. > </para> > > <para> > - By default, <function>pg_start_backup</function> can take a long time to finish. > + By default, <function>pg_backup_start</function> can take a long time to finish. > This is because it performs a checkpoint, and the I/O > required for the checkpoint will be spread out over a significant > period of time, by default half your inter-checkpoint interval Hrmpf. Not this patch's fault, but the default isn't 'half your inter-checkpoint interval' anymore (checkpoint_completion_target was changed to 0.9 ... let's not discuss who did that and missed fixing this). Overall though, maybe we should reword this to avoid having to remember to change it again if we ever change the completion target again? Also it seems to imply that pg_backup_start is going to run its own independent checkpoint or something, which isn't the typical case. I generally explain what is going on here like this: By default, <function>pg_backup_start</function> will wait for the next checkpoint to complete (see ref: checkpoint_timeout ... maybe also wal-configuration.html). > @@ -937,7 +925,7 @@ SELECT * FROM pg_stop_backup(false, true); > ready to archive. > </para> > <para> > - The <function>pg_stop_backup</function> will return one row with three > + The <function>pg_backup_stop</function> will return one row with three > values. The second of these fields should be written to a file named > <filename>backup_label</filename> in the root directory of the backup. The > third field should be written to a file named I get that we have <function> and </function>, but those are just formatting and don't show up in the actual text, and so it ends up being: The pg_backup_stop will return Which doesn't sound quite right to me. I'd say we either drop 'The' or add 'function' after. I realize that this patch is mostly doing a s/pg_stop_backup/pg_backup_stop/, but, still. > diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml > index 8a802fb225..73096708cc 100644 > --- a/doc/src/sgml/func.sgml > +++ b/doc/src/sgml/func.sgml > @@ -25732,24 +25715,19 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560 > <parameter>spcmapfile</parameter> <type>text</type> ) > </para> > <para> > - Finishes performing an exclusive or non-exclusive on-line backup. > - The <parameter>exclusive</parameter> parameter must match the > - previous <function>pg_start_backup</function> call. > - In an exclusive backup, <function>pg_stop_backup</function> removes > - the backup label file and, if it exists, the tablespace map file > - created by <function>pg_start_backup</function>. In a non-exclusive > - backup, the desired contents of these files are returned as part of > + Finishes performing an on-line backup. The desired contents of the > + backup label file and the tablespace map file are returned as part of > the result of the function, and should be written to files in the > backup area (not in the data directory). > </para> Given that this is a crucial part, which the exclusive mode has wrong, I'd be a bit more forceful about it, eg: The contents of the backup label file and the tablespace map file must be stored as part of the backup and must NOT be stored in the live data directory (doing so will cause PostgreSQL to fail to restart in the event of a crash). > @@ -25771,8 +25749,7 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560 > The result of the function is a single record. > The <parameter>lsn</parameter> column holds the backup's ending > write-ahead log location (which again can be ignored). The second and > - third columns are <literal>NULL</literal> when ending an exclusive > - backup; after a non-exclusive backup they hold the desired contents of > + third columns hold the desired contents of > the label and tablespace map files. > </para> > <para> I dislike saying 'desired' here as if it's something that we're nicely asking but maybe isn't a big deal, and just saying 'label' isn't good since we call it 'backup label' elsewhere and we should try hard to be consistent and clear. How about: The second column returns the contents of the backup label file, the third column returns the contents of the tablespace map file. These must be stored as part of the backup and are required as part of the restore process. > diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c > index 3c182c97d4..ee3fa148b6 100644 > --- a/src/bin/pg_ctl/pg_ctl.c > +++ b/src/bin/pg_ctl/pg_ctl.c > @@ -1069,7 +1069,7 @@ do_stop(void) > get_control_dbstate() != DB_IN_ARCHIVE_RECOVERY) > { > print_msg(_("WARNING: online backup mode is active\n" > - "Shutdown will not complete until pg_stop_backup() is called.\n\n")); > + "Shutdown will not complete until pg_backup_stop() is called.\n\n")); > } > > print_msg(_("waiting for server to shut down...")); > @@ -1145,7 +1145,7 @@ do_restart(void) > get_control_dbstate() != DB_IN_ARCHIVE_RECOVERY) > { > print_msg(_("WARNING: online backup mode is active\n" > - "Shutdown will not complete until pg_stop_backup() is called.\n\n")); > + "Shutdown will not complete until pg_backup_stop() is called.\n\n")); > } > > print_msg(_("waiting for server to shut down...")); This... can't actually happen anymore, right? Shouldn't we just pull all of this out? On a once-over of the rest of the code, I definitely like how much we're able to simplify things in this area and remove various hacks in things like pg_basebackup and pg_rewind where we previously had to worry about backup_label and tablespace_map files being in a live data directory. I'm planning to spend more time on this and hopefully be able to get it in for v15. Thanks! Stephen Attachments: [application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2022-03-29 02:09 Nathan Bossart <[email protected]> parent: Stephen Frost <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Nathan Bossart @ 2022-03-29 02:09 UTC (permalink / raw) To: Stephen Frost <[email protected]>; +Cc: Chapman Flack <[email protected]>; Magnus Hagander <[email protected]>; David Steele <[email protected]>; [email protected] On Mon, Mar 28, 2022 at 04:30:27PM -0400, Stephen Frost wrote: >> - By default, <function>pg_start_backup</function> can take a long time to finish. >> + By default, <function>pg_backup_start</function> can take a long time to finish. >> This is because it performs a checkpoint, and the I/O >> required for the checkpoint will be spread out over a significant >> period of time, by default half your inter-checkpoint interval > > Hrmpf. Not this patch's fault, but the default isn't 'half your > inter-checkpoint interval' anymore (checkpoint_completion_target was > changed to 0.9 ... let's not discuss who did that and missed fixing > this). Overall though, maybe we should reword this to avoid having to > remember to change it again if we ever change the completion target > again? Also it seems to imply that pg_backup_start is going to run its > own independent checkpoint or something, which isn't the typical case. > I generally explain what is going on here like this: > > By default, <function>pg_backup_start</function> will wait for the next > checkpoint to complete (see ref: checkpoint_timeout ... maybe also > wal-configuration.html). done >> - The <function>pg_stop_backup</function> will return one row with three >> + The <function>pg_backup_stop</function> will return one row with three >> values. The second of these fields should be written to a file named >> <filename>backup_label</filename> in the root directory of the backup. The >> third field should be written to a file named > > I get that we have <function> and </function>, but those are just > formatting and don't show up in the actual text, and so it ends up > being: > > The pg_backup_stop will return > > Which doesn't sound quite right to me. I'd say we either drop 'The' or > add 'function' after. I realize that this patch is mostly doing a > s/pg_stop_backup/pg_backup_stop/, but, still. done >> - Finishes performing an exclusive or non-exclusive on-line backup. >> - The <parameter>exclusive</parameter> parameter must match the >> - previous <function>pg_start_backup</function> call. >> - In an exclusive backup, <function>pg_stop_backup</function> removes >> - the backup label file and, if it exists, the tablespace map file >> - created by <function>pg_start_backup</function>. In a non-exclusive >> - backup, the desired contents of these files are returned as part of >> + Finishes performing an on-line backup. The desired contents of the >> + backup label file and the tablespace map file are returned as part of >> the result of the function, and should be written to files in the >> backup area (not in the data directory). >> </para> > > Given that this is a crucial part, which the exclusive mode has wrong, > I'd be a bit more forceful about it, eg: > > The contents of the backup label file and the tablespace map file must > be stored as part of the backup and must NOT be stored in the live data > directory (doing so will cause PostgreSQL to fail to restart in the > event of a crash). done >> The result of the function is a single record. >> The <parameter>lsn</parameter> column holds the backup's ending >> write-ahead log location (which again can be ignored). The second and >> - third columns are <literal>NULL</literal> when ending an exclusive >> - backup; after a non-exclusive backup they hold the desired contents of >> + third columns hold the desired contents of >> the label and tablespace map files. >> </para> >> <para> > > I dislike saying 'desired' here as if it's something that we're nicely > asking but maybe isn't a big deal, and just saying 'label' isn't good > since we call it 'backup label' elsewhere and we should try hard to be > consistent and clear. How about: > > The second column returns the contents of the backup label file, the > third column returns the contents of the tablespace map file. These > must be stored as part of the backup and are required as part of the > restore process. done >> { >> print_msg(_("WARNING: online backup mode is active\n" >> - "Shutdown will not complete until pg_stop_backup() is called.\n\n")); >> + "Shutdown will not complete until pg_backup_stop() is called.\n\n")); >> } >> >> print_msg(_("waiting for server to shut down...")); >> @@ -1145,7 +1145,7 @@ do_restart(void) >> get_control_dbstate() != DB_IN_ARCHIVE_RECOVERY) >> { >> print_msg(_("WARNING: online backup mode is active\n" >> - "Shutdown will not complete until pg_stop_backup() is called.\n\n")); >> + "Shutdown will not complete until pg_backup_stop() is called.\n\n")); >> } >> >> print_msg(_("waiting for server to shut down...")); > > This... can't actually happen anymore, right? Shouldn't we just pull > all of this out? done > On a once-over of the rest of the code, I definitely like how much we're > able to simplify things in this area and remove various hacks in things > like pg_basebackup and pg_rewind where we previously had to worry about > backup_label and tablespace_map files being in a live data directory. > I'm planning to spend more time on this and hopefully be able to get it > in for v15. Great! Much appreciated. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file @ 2022-04-20 02:06 Michael Paquier <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 28+ messages in thread From: Michael Paquier @ 2022-04-20 02:06 UTC (permalink / raw) To: Martín Marqués <[email protected]>; +Cc: Stephen Frost <[email protected]>; Laurenz Albe <[email protected]>; Nathan Bossart <[email protected]>; David Steele <[email protected]>; Chapman Flack <[email protected]>; Magnus Hagander <[email protected]>; [email protected] On Tue, Apr 19, 2022 at 10:12:32PM -0300, Martín Marqués wrote: > The typo is in `exist in in a running cluster`. There's two `in` in a row. Thanks, fixed. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 28+ messages in thread
end of thread, other threads:[~2022-04-20 02:06 UTC | newest] Thread overview: 28+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-11-24 22:12 Postgres restart in the middle of exclusive backup and the presence of backup_label file SATYANARAYANA NARLAPURAM <[email protected]> 2021-11-24 22:45 ` Michael Paquier <[email protected]> 2021-11-26 02:19 ` SATYANARAYANA NARLAPURAM <[email protected]> 2021-11-26 05:57 ` Michael Paquier <[email protected]> 2021-11-26 15:31 ` Tom Lane <[email protected]> 2021-11-29 18:25 ` Bossart, Nathan <[email protected]> 2021-11-30 14:20 ` Stephen Frost <[email protected]> 2021-11-30 16:47 ` Laurenz Albe <[email protected]> 2021-11-30 17:48 ` Stephen Frost <[email protected]> 2021-11-30 21:56 ` Bossart, Nathan <[email protected]> 2021-11-30 22:26 ` Tom Lane <[email protected]> 2021-11-30 22:50 ` Bossart, Nathan <[email protected]> 2021-11-30 22:58 ` David Steele <[email protected]> 2021-11-30 23:31 ` Bossart, Nathan <[email protected]> 2021-12-01 16:27 ` David Steele <[email protected]> 2021-12-01 18:33 ` Bossart, Nathan <[email protected]> 2021-12-02 00:30 ` Bossart, Nathan <[email protected]> 2021-12-02 16:00 ` Andrew Dunstan <[email protected]> 2021-12-02 17:38 ` David Steele <[email protected]> 2021-12-02 17:49 ` David Steele <[email protected]> 2021-12-02 21:31 ` Bossart, Nathan <[email protected]> 2021-12-01 00:54 ` Michael Paquier <[email protected]> 2021-12-01 01:56 ` SATYANARAYANA NARLAPURAM <[email protected]> 2021-12-01 16:00 ` David Steele <[email protected]> 2021-12-01 16:14 ` Andrew Dunstan <[email protected]> 2022-03-28 20:30 Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file Stephen Frost <[email protected]> 2022-03-29 02:09 ` Nathan Bossart <[email protected]> 2022-04-20 02:06 ` Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox