Rename backup_label to recovery_control

Started by David Steeleover 2 years ago14 messageshackers
Jump to latest
#1David Steele
david@pgmasters.net

Hackers,

I was recently discussing the complexities of dealing with pg_control
and backup_label with some hackers at PGConf NYC, when David Christensen
commented that backup_label was not a very good name since it gives the
impression of being informational and therefore something the user can
delete. In fact, we see this happen quite a lot, and there have been
some other discussions about it recently, see [1]/messages/by-id/1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net and [2]/messages/by-id/CAM_vCudkSjr7NsNKSdjwtfAm9dbzepY6beZ5DP177POKy8=2aw@mail.gmail.com. I bounced the
idea of a rename off various hackers at the conference and in general
people seemed to think it was a good idea.

Attached is a patch to rename backup_label to recovery_control. The
purpose is to make it more obvious that the file should not be deleted.
I'm open to other names, e.g. recovery.control. That makes the naming
distinct from tablespace_map, which is perhaps a good thing, but is also
more likely to be confused with recovery.signal.

I did a pretty straight-forward search and replace on comments and
documentation with only light editing. If this seems like a good idea
and we choose a final name, I'll do a more thorough pass through the
comments and docs to try and make the usage more consistent.

Note that there is one usage of backup label that remains, i.e. the text
that the user can set to describe the backup.

Regards,
-David

[1]: /messages/by-id/1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net
/messages/by-id/1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net
[2]: /messages/by-id/CAM_vCudkSjr7NsNKSdjwtfAm9dbzepY6beZ5DP177POKy8=2aw@mail.gmail.com
/messages/by-id/CAM_vCudkSjr7NsNKSdjwtfAm9dbzepY6beZ5DP177POKy8=2aw@mail.gmail.com

Attachments:

v01-rename-backup-label-to-recovery-control.patchtext/plain; charset=UTF-8; name=v01-rename-backup-label-to-recovery-control.patchDownload+98-96
#2Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: David Steele (#1)
Re: Rename backup_label to recovery_control

At Sat, 14 Oct 2023 14:19:42 -0400, David Steele <david@pgmasters.net> wrote in

I was recently discussing the complexities of dealing with pg_control
and backup_label with some hackers at PGConf NYC, when David
Christensen commented that backup_label was not a very good name since
it gives the impression of being informational and therefore something
the user can delete. In fact, we see this happen quite a lot, and
there have been some other discussions about it recently, see [1] and
[2]. I bounced the idea of a rename off various hackers at the
conference and in general people seemed to think it was a good idea.

Attached is a patch to rename backup_label to recovery_control. The

Just an idea in a slightly different direction, but I'm wondering if
we can simply merge the content of backup_label into control file.
The file is 8192 bytes long, yet only 256 bytes are used. As a result,
we anticipate no overhead. Sucha configuration would forcibly prevent
uses from from removing the backup information.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#3Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Kyotaro Horiguchi (#2)
Re: Rename backup_label to recovery_control

At Mon, 16 Oct 2023 13:16:42 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

Just an idea in a slightly different direction, but I'm wondering if
we can simply merge the content of backup_label into control file.
The file is 8192 bytes long, yet only 256 bytes are used. As a result,
we anticipate no overhead. Sucha configuration would forcibly prevent
uses from from removing the backup information.

In second thought, that would break the case of file-system level
backups, which require backup information separately from control
data.

Sorry for the noise.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#4Michael Paquier
michael@paquier.xyz
In reply to: Kyotaro Horiguchi (#2)
Re: Rename backup_label to recovery_control

On Mon, Oct 16, 2023 at 01:16:42PM +0900, Kyotaro Horiguchi wrote:

Just an idea in a slightly different direction, but I'm wondering if
we can simply merge the content of backup_label into control file.
The file is 8192 bytes long, yet only 256 bytes are used. As a result,
we anticipate no overhead. Sucha configuration would forcibly prevent
uses from from removing the backup information.

With the critical assumptions behind PG_CONTROL_MAX_SAFE_SIZE, that
does not sound like a good idea to me. And that's without the fact
that base backup labels could make the control file theoretically even
larger than PG_CONTROL_FILE_SIZE, even if that's unlikely going to
happen in practice.
--
Michael

#5David Steele
david@pgmasters.net
In reply to: Kyotaro Horiguchi (#3)
Re: Rename backup_label to recovery_control

On 10/16/23 00:26, Kyotaro Horiguchi wrote:

At Mon, 16 Oct 2023 13:16:42 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

Just an idea in a slightly different direction, but I'm wondering if
we can simply merge the content of backup_label into control file.
The file is 8192 bytes long, yet only 256 bytes are used. As a result,
we anticipate no overhead. Sucha configuration would forcibly prevent
uses from from removing the backup information.

In second thought, that would break the case of file-system level
backups, which require backup information separately from control
data.

Exactly -- but we do have a proposal to do the opposite and embed
pg_control into backup_label [1]/messages/by-id/1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net (or hopefully recovery_control).

Regards,
-David

[1]: /messages/by-id/1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net
/messages/by-id/1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net

#6Robert Haas
robertmhaas@gmail.com
In reply to: David Steele (#1)
Re: Rename backup_label to recovery_control

On Sat, Oct 14, 2023 at 2:22 PM David Steele <david@pgmasters.net> wrote:

I was recently discussing the complexities of dealing with pg_control
and backup_label with some hackers at PGConf NYC, when David Christensen
commented that backup_label was not a very good name since it gives the
impression of being informational and therefore something the user can
delete. In fact, we see this happen quite a lot, and there have been
some other discussions about it recently, see [1] and [2]. I bounced the
idea of a rename off various hackers at the conference and in general
people seemed to think it was a good idea.

Personally, I feel like this is an area where we keep moving the parts
around but I'm not sure we're really getting to anything better. We
got rid of recovery.conf. We got rid of exclusive backup mode. We
replaced pg_start_backup with pg_backup_start. It feels like every
other release or so we whack something around here, but I'm not
convinced that any of it is really making much of an impact. If
there's been any decrease in people screwing up their backups, I
haven't noticed it.

To be fair, I will grant that renaming pg_clog to pg_xact_status and
pg_xlog to pg_wal does seem to have reduced the incidence of people
nuking those directories, at least IME. So maybe this change would
help too, for similar reasons. But I'm still concerned that we're
doing too much superficial tinkering in this area. Breaking
compatibility is not without cost.

I also do wonder with recovery_control is really a better name. Maybe
I just have backup_label too firmly stuck in my head, but is what that
file does really best described as recovery control? I'm not so sure
about that.

--
Robert Haas
EDB: http://www.enterprisedb.com

#7David Steele
david@pgmasters.net
In reply to: Robert Haas (#6)
Re: Rename backup_label to recovery_control

On 10/16/23 10:19, Robert Haas wrote:

On Sat, Oct 14, 2023 at 2:22 PM David Steele <david@pgmasters.net> wrote:

I was recently discussing the complexities of dealing with pg_control
and backup_label with some hackers at PGConf NYC, when David Christensen
commented that backup_label was not a very good name since it gives the
impression of being informational and therefore something the user can
delete. In fact, we see this happen quite a lot, and there have been
some other discussions about it recently, see [1] and [2]. I bounced the
idea of a rename off various hackers at the conference and in general
people seemed to think it was a good idea.

Personally, I feel like this is an area where we keep moving the parts
around but I'm not sure we're really getting to anything better. We
got rid of recovery.conf.

I agree that this was not an improvement. I was fine with bringing the
recovery options into the GUC fold but never really liked forcing them
into postgresql.auto.conf. But I lost that argument.

We got rid of exclusive backup mode. We
replaced pg_start_backup with pg_backup_start.

I do think this was an improvement. For example it allows us to do [1]/messages/by-id/1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net,
which I believe is a better overall solution to the problem of torn
reads of pg_control. With exclusive backup we would not have this option.

It feels like every
other release or so we whack something around here, but I'm not
convinced that any of it is really making much of an impact. If
there's been any decrease in people screwing up their backups, I
haven't noticed it.

It's pretty subjective, but I feel much the same way. However, I think
the *areas* that people are messing up are changing as we remove
obstacles and I feel like we should address them. backup_label has
always been a bit of a problem -- basically deciding should it be deleted?

With the removal of exclusive backup we removed the only valid use case
(I think) for removing backup_label manually. Now, it should probably
never be removed manually, so we need to make adjustments to make that
clearer to the user, also see [1]/messages/by-id/1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net.

Better messaging may also help, and I am also thinking about that.

To be fair, I will grant that renaming pg_clog to pg_xact_status and
pg_xlog to pg_wal does seem to have reduced the incidence of people
nuking those directories, at least IME. So maybe this change would
help too, for similar reasons. But I'm still concerned that we're
doing too much superficial tinkering in this area. Breaking
compatibility is not without cost.

True enough, but ISTM that we have gotten few (or any) actual complaints
outside of hackers speculating that there will be complaints. For the
various maintainers of backup software this is just business as usual.
The changes to pg_basebackup are also pretty trivial.

I also do wonder with recovery_control is really a better name. Maybe
I just have backup_label too firmly stuck in my head, but is what that
file does really best described as recovery control? I'm not so sure
about that.

The thing it does that describes it as "recovery control" in my view is
that it contains the LSN where Postgres must start recovery (plus TLI,
backup method, etc.). There is some other informational stuff in there,
but the important fields are all about ensuring consistent recovery.

At the end of the day the entire point of backup *is* recovery and users
will interact with this file primarily in recovery scenarios.

Regards,
-David

---

[1]: /messages/by-id/1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net
/messages/by-id/1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net

#8Michael Banck
michael.banck@credativ.de
In reply to: David Steele (#7)
Re: Rename backup_label to recovery_control

On Mon, Oct 16, 2023 at 11:15:53AM -0400, David Steele wrote:

On 10/16/23 10:19, Robert Haas wrote:

We got rid of exclusive backup mode. We replaced pg_start_backup
with pg_backup_start.

I do think this was an improvement. For example it allows us to do
[1], which I believe is a better overall solution to the problem of
torn reads of pg_control. With exclusive backup we would not have this
option.

Well maybe, but it also seems to mean that any other 3rd party (i.e. not
Postgres-specific) backup tool seems to only support Postgres up till
version 14, as they cannot deal with non-exclusive mode - they are used
to a simple pre/post-script approach.

Not sure what to do about this, but as people/companies start moving to
15, I am afraid we will get people complaining about this. I think
having exclusive mode still be the default for pg_start_backup() (albeit
deprecated) in one release and then dropping it in the next was too
fast.

Or is somebody helping those "enterprise" backup solutions along in
implementing non-exclusive Postgres backups?

Michael

#9Robert Haas
robertmhaas@gmail.com
In reply to: Michael Banck (#8)
Re: Rename backup_label to recovery_control

On Mon, Oct 16, 2023 at 12:06 PM Michael Banck <mbanck@gmx.net> wrote:

Not sure what to do about this, but as people/companies start moving to
15, I am afraid we will get people complaining about this. I think
having exclusive mode still be the default for pg_start_backup() (albeit
deprecated) in one release and then dropping it in the next was too
fast.

I completely agree, and I said so at the time, but got shouted down. I
think the argument that exclusive backups were breaking anything at
all was very weak. Nobody was being forced to use them, and they broke
nothing for people who didn't.

--
Robert Haas
EDB: http://www.enterprisedb.com

#10David Steele
david@pgmasters.net
In reply to: Michael Banck (#8)
Re: Rename backup_label to recovery_control

On 10/16/23 12:06, Michael Banck wrote:

On Mon, Oct 16, 2023 at 11:15:53AM -0400, David Steele wrote:

On 10/16/23 10:19, Robert Haas wrote:

We got rid of exclusive backup mode. We replaced pg_start_backup
with pg_backup_start.

I do think this was an improvement. For example it allows us to do
[1], which I believe is a better overall solution to the problem of
torn reads of pg_control. With exclusive backup we would not have this
option.

Well maybe, but it also seems to mean that any other 3rd party (i.e. not
Postgres-specific) backup tool seems to only support Postgres up till
version 14, as they cannot deal with non-exclusive mode - they are used
to a simple pre/post-script approach.

I'd be curious to know what enterprise solutions currently depend on
this method. At the very least they'd need to manage a WAL archive since
copying pg_wal is not a safe thing to do (without a snapshot), so it's
not just a matter of using start/stop scripts. And you'd probably want
PITR, etc.

Not sure what to do about this, but as people/companies start moving to
15, I am afraid we will get people complaining about this. I think
having exclusive mode still be the default for pg_start_backup() (albeit
deprecated) in one release and then dropping it in the next was too
fast.

But lots of companies are on PG15 and lots of hosting providers support
it, apparently with no issues. Perhaps the companies you are referring
to are lagging in adoption (a pretty common scenario) but I still see no
evidence that there is a big problem looming.

Exclusive backup was deprecated for six releases, which should have been
ample time to switch over. All the backup solutions I am familiar with
have supported non-exclusive backup for years.

Or is somebody helping those "enterprise" backup solutions along in
implementing non-exclusive Postgres backups?

I couldn't say, but there are many examples in open source projects of
how to do this. Somebody (Laurenz, I believe) also wrote a shell script
to simulate exclusive backup behavior for those that want to continue
using it. Not what I would recommend, but he showed that it was possible.

Regards,
-David

#11David Steele
david@pgmasters.net
In reply to: Robert Haas (#9)
Re: Rename backup_label to recovery_control

On 10/16/23 12:12, Robert Haas wrote:

On Mon, Oct 16, 2023 at 12:06 PM Michael Banck <mbanck@gmx.net> wrote:

Not sure what to do about this, but as people/companies start moving to
15, I am afraid we will get people complaining about this. I think
having exclusive mode still be the default for pg_start_backup() (albeit
deprecated) in one release and then dropping it in the next was too
fast.

I completely agree, and I said so at the time, but got shouted down. I
think the argument that exclusive backups were breaking anything at
all was very weak. Nobody was being forced to use them, and they broke
nothing for people who didn't.

My argument then (and now) is that exclusive backup prevented us from
making material improvements in backup and recovery. It was complicated,
duplicative (in code and docs), and entirely untested.

So you are correct that it was only dangerous to the people who were
using it (even if they did not know they were), but it was also a
barrier to progress.

Regards,
-David

#12Laurenz Albe
laurenz.albe@cybertec.at
In reply to: Robert Haas (#9)
Re: Rename backup_label to recovery_control

On Mon, 2023-10-16 at 12:12 -0400, Robert Haas wrote:

On Mon, Oct 16, 2023 at 12:06 PM Michael Banck <mbanck@gmx.net> wrote:

Not sure what to do about this, but as people/companies start moving to
15, I am afraid we will get people complaining about this. I think
having exclusive mode still be the default for pg_start_backup() (albeit
deprecated) in one release and then dropping it in the next was too
fast.

I completely agree, and I said so at the time, but got shouted down. I
think the argument that exclusive backups were breaking anything at
all was very weak. Nobody was being forced to use them, and they broke
nothing for people who didn't.

+1

Yours,
Laurenz Albe

#13Peter Eisentraut
peter_e@gmx.net
In reply to: David Steele (#7)
Re: Rename backup_label to recovery_control

On 16.10.23 17:15, David Steele wrote:

I also do wonder with recovery_control is really a better name. Maybe
I just have backup_label too firmly stuck in my head, but is what that
file does really best described as recovery control? I'm not so sure
about that.

The thing it does that describes it as "recovery control" in my view is
that it contains the LSN where Postgres must start recovery (plus TLI,
backup method, etc.). There is some other informational stuff in there,
but the important fields are all about ensuring consistent recovery.

At the end of the day the entire point of backup *is* recovery and users
will interact with this file primarily in recovery scenarios.

Maybe "restore" is better than "recovery", since recovery also happens
separate from backups, but restoring is something you do with a backup
(and there is also restore_command etc.).

#14David Steele
david@pgmasters.net
In reply to: Peter Eisentraut (#13)
Re: Rename backup_label to recovery_control

On 10/18/23 03:07, Peter Eisentraut wrote:

On 16.10.23 17:15, David Steele wrote:

I also do wonder with recovery_control is really a better name. Maybe
I just have backup_label too firmly stuck in my head, but is what that
file does really best described as recovery control? I'm not so sure
about that.

The thing it does that describes it as "recovery control" in my view
is that it contains the LSN where Postgres must start recovery (plus
TLI, backup method, etc.). There is some other informational stuff in
there, but the important fields are all about ensuring consistent
recovery.

At the end of the day the entire point of backup *is* recovery and
users will interact with this file primarily in recovery scenarios.

Maybe "restore" is better than "recovery", since recovery also happens
separate from backups, but restoring is something you do with a backup
(and there is also restore_command etc.).

I would not object to restore (there is restore_command) but I do think
of what PostgreSQL does as "recovery" as opposed to "restore", which
comes before the recovery. Recovery is used a lot in the docs and there
is also recovery.signal.

But based on the discussion in [1]/messages/by-id/0f948866-7caf-0759-d53c-93c3e266ec3f@pgmasters.net I think we might be able to do away
with backup_label entirely, which would make this change moot.

Regards,
-David

[1]: /messages/by-id/0f948866-7caf-0759-d53c-93c3e266ec3f@pgmasters.net
/messages/by-id/0f948866-7caf-0759-d53c-93c3e266ec3f@pgmasters.net