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

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Wed, 24 Nov 2021 14:12:19 -0800

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

---

From: Michael Paquier <[email-masked]>
Date: Thu, 25 Nov 2021 07:45:46 +0900

> don't see a reason why everyone has to repeat this step. Am I missing

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

---

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Thu, 25 Nov 2021 18:19:03 -0800

Thanks Michael!

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

---

From: Michael Paquier <[email-masked]>
Date: Fri, 26 Nov 2021 14:57:14 +0900

On Thu, Nov 25, 2021 at 06:19:03PM -0800, SATYANARAYANA NARLAPURAM wrote:
> 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

---

From: Tom Lane <[email-masked]>
Date: Fri, 26 Nov 2021 10:31:00 -0500

> 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

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 29 Nov 2021 18:25:24 +0000

> 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

---

From: Stephen Frost <[email-masked]>
Date: Tue, 30 Nov 2021 09:20:42 -0500

Greetings,

> 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

---

From: Laurenz Albe <[email-masked]>
Date: Tue, 30 Nov 2021 17:47:18 +0100

On Tue, 2021-11-30 at 09:20 -0500, Stephen Frost wrote:
> 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.

> remove exclusive backup in a major version change without the world

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

---

From: Stephen Frost <[email-masked]>
Date: Tue, 30 Nov 2021 12:48:15 -0500

Greetings,

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

> 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

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 30 Nov 2021 21:56:36 +0000

On 11/30/21, 9:51 AM, "Stephen Frost" <[email-masked]> wrote:
> 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

---

From: Tom Lane <[email-masked]>
Date: Tue, 30 Nov 2021 17:26:07 -0500

> 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

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 30 Nov 2021 22:50:11 +0000

> but removing it isn't that.

I might try to put a patch together for the January commitfest, given
there is enough support.

Nathan

---

From: David Steele <[email-masked]>
Date: Tue, 30 Nov 2021 17:58:15 -0500

> 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

---

From: "Bossart, Nathan" <[email-masked]>
Date: Tue, 30 Nov 2021 23:31:04 +0000

> 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

Ah, good, some light reading.  :)

Nathan

---

From: Michael Paquier <[email-masked]>
Date: Wed, 1 Dec 2021 09:54:18 +0900

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

---

From: SATYANARAYANA NARLAPURAM <[email-masked]>
Date: Tue, 30 Nov 2021 17:56:39 -0800

On Tue, Nov 30, 2021 at 4:54 PM Michael Paquier <[email-masked]> wrote:

> backup
> having

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.

---

From: David Steele <[email-masked]>
Date: Wed, 1 Dec 2021 11:00:20 -0500

> 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

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 1 Dec 2021 11:14:54 -0500

> 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

---

From: David Steele <[email-masked]>
Date: Wed, 1 Dec 2021 11:27:42 -0500

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

> Ah, good, some light reading.  :)

Sure, if you say so!

Regards,
-David

---

From: "Bossart, Nathan" <[email-masked]>
Date: Wed, 1 Dec 2021 18:33:29 +0000

> My preference would be to remove it completely, but I haven't gotten a

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

---

From: "Bossart, Nathan" <[email-masked]>
Date: Thu, 2 Dec 2021 00:30:25 +0000

> 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

---

From: Andrew Dunstan <[email-masked]>
Date: Thu, 2 Dec 2021 11:00:32 -0500

> 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

---

From: David Steele <[email-masked]>
Date: Thu, 2 Dec 2021 12:38:50 -0500

> 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

---

From: David Steele <[email-masked]>
Date: Thu, 2 Dec 2021 12:49:52 -0500

> mode and save out backup_label instead.

Oops, of course I meant non-exclusive mode.

Regards,
-David

---

From: "Bossart, Nathan" <[email-masked]>
Date: Thu, 2 Dec 2021 21:31:53 +0000

> Oops, of course I meant non-exclusive mode.

+1.  I'll fix that in the next revision.

Nathan