Thread: The danger of deleting backup_label

The danger of deleting backup_label

From
David Steele
Date:
Hackers,

While reading through [1] I saw there were two instances where 
backup_label was removed to achieve a "successful" restore. This might 
work on trivial test restores but is an invitation to (silent) disaster 
in a production environment where the checkpoint stored in backup_label 
is almost certain to be earlier than the one stored in pg_control.

A while back I had an idea on how to prevent this so I decided to give 
it a try. Basically, before writing pg_control to the backup I set 
checkpoint to 0xFFFFFFFFFFFFFFFF.

Recovery worked perfectly as long as backup_label was present and failed 
hard when it was not:

LOG:  invalid primary checkpoint record
PANIC:  could not locate a valid checkpoint record

It's not a very good message, but at least the foot gun has been 
removed. We could use this as a special value to give a better message, 
and maybe use something a bit more unique like 0xFFFFFFFFFADEFADE (or 
whatever) as the value.

This is all easy enough for pg_basebackup to do, but will certainly be 
non-trivial for most backup software to implement. In [2] we have 
discussed perhaps returning pg_control from pg_backup_stop() for the 
backup software to save, or it could become part of the backup_label 
(encoded as hex or base64, presumably). I prefer the latter as this 
means less work for the backup software (except for the need to exclude 
pg_control from the backup).

I don't have a patch for this yet because I did not test this idea using 
pg_basebackup, but I'll be happy to work up a patch if there is interest.

I feel like we should do *something* here. If even advanced users are 
making this mistake, then we should take it pretty seriously.

Regards,
-David

[1] 

https://www.postgresql.org/message-id/flat/CAM_vCudkSjr7NsNKSdjwtfAm9dbzepY6beZ5DP177POKy8%3D2aw%40mail.gmail.com#746e492bfcd2667635634f1477a61288
[2] 
https://www.postgresql.org/message-id/CA%2BhUKGKiZJcfZSA5G5Rm8oC78SNOQ4c8az5Ku%3D4wMTjw1FZ40g%40mail.gmail.com



Re: The danger of deleting backup_label

From
Michael Paquier
Date:
On Thu, Sep 28, 2023 at 05:14:22PM -0400, David Steele wrote:
> While reading through [1] I saw there were two instances where backup_label
> was removed to achieve a "successful" restore. This might work on trivial
> test restores but is an invitation to (silent) disaster in a production
> environment where the checkpoint stored in backup_label is almost certain to
> be earlier than the one stored in pg_control.

Definitely successful.

> Recovery worked perfectly as long as backup_label was present and failed
> hard when it was not:
>
> LOG:  invalid primary checkpoint record
> PANIC:  could not locate a valid checkpoint record
>
> It's not a very good message, but at least the foot gun has been removed. We
> could use this as a special value to give a better message, and maybe use
> something a bit more unique like 0xFFFFFFFFFADEFADE (or whatever) as the
> value.

Why not just InvalidXLogRecPtr?

> This is all easy enough for pg_basebackup to do, but will certainly be
> non-trivial for most backup software to implement. In [2] we have discussed
> perhaps returning pg_control from pg_backup_stop() for the backup software
> to save, or it could become part of the backup_label (encoded as hex or
> base64, presumably). I prefer the latter as this means less work for the
> backup software (except for the need to exclude pg_control from the backup).
>
> I don't have a patch for this yet because I did not test this idea using
> pg_basebackup, but I'll be happy to work up a patch if there is interest.

If the contents of the control file are tweaked before sending it
through a BASE_BACKUP, it would cover more than just pg_basebackup.
Switching the way the control file is sent with new contents in
sendFileWithContent() rather than sendFile() would be one way, for
instance..
--
Michael

Attachment

Re: The danger of deleting backup_label

From
David Steele
Date:
On 9/28/23 22:30, Michael Paquier wrote:
> On Thu, Sep 28, 2023 at 05:14:22PM -0400, David Steele wrote:
> 
>> Recovery worked perfectly as long as backup_label was present and failed
>> hard when it was not:
>>
>> LOG:  invalid primary checkpoint record
>> PANIC:  could not locate a valid checkpoint record
>>
>> It's not a very good message, but at least the foot gun has been removed. We
>> could use this as a special value to give a better message, and maybe use
>> something a bit more unique like 0xFFFFFFFFFADEFADE (or whatever) as the
>> value.
> 
> Why not just InvalidXLogRecPtr?

That fails because there is a check to make sure the checkpoint is valid 
when pg_control is loaded. Another possibility is to use a special LSN 
like we use for unlogged tables. Anything >= 24 and < WAL segment size 
will work fine.

>> This is all easy enough for pg_basebackup to do, but will certainly be
>> non-trivial for most backup software to implement. In [2] we have discussed
>> perhaps returning pg_control from pg_backup_stop() for the backup software
>> to save, or it could become part of the backup_label (encoded as hex or
>> base64, presumably). I prefer the latter as this means less work for the
>> backup software (except for the need to exclude pg_control from the backup).
>>
>> I don't have a patch for this yet because I did not test this idea using
>> pg_basebackup, but I'll be happy to work up a patch if there is interest.
> 
> If the contents of the control file are tweaked before sending it
> through a BASE_BACKUP, it would cover more than just pg_basebackup.
> Switching the way the control file is sent with new contents in
> sendFileWithContent() rather than sendFile() would be one way, for
> instance..

Good point, and that makes this even more compelling. If we include 
pg_control into backup_label then there is no need to modify pg_control 
(as above) -- we can just exclude it from the backup entirely. That will 
certainly require some rejigging in recovery but seems worth it for 
backup solutions that can't easily modify pg_control. The C-based 
solutions can do this pretty easily but it is a pretty high bar for 
anyone else.

Regards,
-David



Re: The danger of deleting backup_label

From
Thomas Munro
Date:
Hi David,

Even though I spent a whole bunch of time trying to figure out how to
make concurrent reads of the control file sufficiently atomic for
backups (pg_basebackup and low level filesystem tools), and we
explored multiple avenues with varying results, and finally came up
with something that basically works pretty well... actually I just
hate all of that stuff, and I'm hoping to be able to just withdraw
https://commitfest.postgresql.org/45/4025/ and chalk it all up to
discovery/education and call *this* thread the real outcome of that
preliminary work.

So I'm +1 on the idea of putting a control file image into the backup
label and I'm happy that you're looking into it.

We could just leave the control file out of the base backup
completely, as you said, removing a whole foot-gun.  People following
the 'low level' instructions will still get a copy of the control file
from the filesystem, and I don't see any reliable way to poison that
file without also making it so that a crash wouldn't also be prevented
from recovering.  I have wondered about putting extra "fingerprint"
information into the control file such as the file's path and inode
number etc, so that you can try to distinguish between a control file
written by PostgreSQL, and a control file copied somewhere else, but
that all feels too fragile, and at the end of the day, people
following the low level backup instructions had better follow the low
level backup instructions (hopefully via the intermediary of an
excellent external backup tool).

As Stephen mentioned[1], we could perhaps also complain if both backup
label and control file exist, and then hint that the user should
remove the *control file* (not the backup label!).  I had originally
suggested we would just overwrite the control file, but by explicitly
complaining about it we would also bring the matter to tool/script
authors' attention, ie that they shouldn't be backing that file up, or
should be removing it in a later step if they copy everything.  He
also mentions that there doesn't seem to be anything stopping us from
back-patching changes to the backup label contents if we go this way.
I don't have a strong opinion on that and we could leave the question
for later.

[1] https://www.postgresql.org/message-id/ZL69NXjCNG%2BWHCqG%40tamriel.snowman.net



Re: The danger of deleting backup_label

From
Michael Paquier
Date:
On Tue, Oct 10, 2023 at 05:06:45PM -0400, David Steele wrote:
> That fails because there is a check to make sure the checkpoint is valid
> when pg_control is loaded. Another possibility is to use a special LSN like
> we use for unlogged tables. Anything >= 24 and < WAL segment size will work
> fine.

Do we have any reason to do that in the presence of a backup_label
file anyway?  We'll know the LSN of the checkpoint based on what the
base backup wants us to use.  Using a fake-still-rather-valid value
for the LSN in the control file to bypass this check does not address
the issue you are pointing at: it is just avoiding this check.  A
reasonable answer would be, IMO, to just not do this check at all
based on the control file in this case.

>> If the contents of the control file are tweaked before sending it
>> through a BASE_BACKUP, it would cover more than just pg_basebackup.
>> Switching the way the control file is sent with new contents in
>> sendFileWithContent() rather than sendFile() would be one way, for
>> instance..
>
> Good point, and that makes this even more compelling. If we include
> pg_control into backup_label then there is no need to modify pg_control (as
> above) -- we can just exclude it from the backup entirely. That will
> certainly require some rejigging in recovery but seems worth it for backup
> solutions that can't easily modify pg_control. The C-based solutions can do
> this pretty easily but it is a pretty high bar for anyone else.

I have little idea about that, but I guess that you are referring to
backrest here.
--
Michael

Attachment

Re: The danger of deleting backup_label

From
David Steele
Date:
Hi Thomas,

On 10/11/23 18:10, Thomas Munro wrote:
> 
> Even though I spent a whole bunch of time trying to figure out how to
> make concurrent reads of the control file sufficiently atomic for
> backups (pg_basebackup and low level filesystem tools), and we
> explored multiple avenues with varying results, and finally came up
> with something that basically works pretty well... actually I just
> hate all of that stuff, and I'm hoping to be able to just withdraw
> https://commitfest.postgresql.org/45/4025/ and chalk it all up to
> discovery/education and call *this* thread the real outcome of that
> preliminary work.
> 
> So I'm +1 on the idea of putting a control file image into the backup
> label and I'm happy that you're looking into it.

Well, hopefully this thread will *at least* be the solution going 
forward. Not sure about a back patch yet, see below...

> We could just leave the control file out of the base backup
> completely, as you said, removing a whole foot-gun.  

That's the plan.

> People following
> the 'low level' instructions will still get a copy of the control file
> from the filesystem, and I don't see any reliable way to poison that
> file without also making it so that a crash wouldn't also be prevented
> from recovering.  I have wondered about putting extra "fingerprint"
> information into the control file such as the file's path and inode
> number etc, so that you can try to distinguish between a control file
> written by PostgreSQL, and a control file copied somewhere else, but
> that all feels too fragile, and at the end of the day, people
> following the low level backup instructions had better follow the low
> level backup instructions (hopefully via the intermediary of an
> excellent external backup tool).

Not sure about the inode idea, because it seems OK for people to move a 
cluster elsewhere under a variety of circumstances. I do have an idea 
about how to mark a cluster in "recovery to consistency" mode, but not 
quite sure how to atomically turn that off at the end of recovery to 
consistency. I have some ideas I'll work on though.

> As Stephen mentioned[1], we could perhaps also complain if both backup
> label and control file exist, and then hint that the user should
> remove the *control file* (not the backup label!).  I had originally
> suggested we would just overwrite the control file, but by explicitly
> complaining about it we would also bring the matter to tool/script
> authors' attention, ie that they shouldn't be backing that file up, or
> should be removing it in a later step if they copy everything.  He
> also mentions that there doesn't seem to be anything stopping us from
> back-patching changes to the backup label contents if we go this way.
> I don't have a strong opinion on that and we could leave the question
> for later.

I'm worried about the possibility of back patching this unless the 
solution comes out to be simpler than I think and that rarely comes to 
pass. Surely throwing errors on something that is currently valid (i.e. 
backup_label and pg_control both present).

But perhaps there is a simpler, acceptable solution we could back patch 
(transparent to all parties except Postgres) and then a more advanced 
solution we could go forward with.

I guess I had better get busy on this.

Regards,
-David

[1] 
https://www.postgresql.org/message-id/ZL69NXjCNG%2BWHCqG%40tamriel.snowman.net



Re: The danger of deleting backup_label

From
David Steele
Date:

On 10/11/23 18:22, Michael Paquier wrote:
> On Tue, Oct 10, 2023 at 05:06:45PM -0400, David Steele wrote:
>> That fails because there is a check to make sure the checkpoint is valid
>> when pg_control is loaded. Another possibility is to use a special LSN like
>> we use for unlogged tables. Anything >= 24 and < WAL segment size will work
>> fine.
> 
> Do we have any reason to do that in the presence of a backup_label
> file anyway?  We'll know the LSN of the checkpoint based on what the
> base backup wants us to use.  Using a fake-still-rather-valid value
> for the LSN in the control file to bypass this check does not address
> the issue you are pointing at: it is just avoiding this check.  A
> reasonable answer would be, IMO, to just not do this check at all
> based on the control file in this case.

Yeah, that's fair. And it looks like we are leaning towards excluding 
pg_control from the backup entirely, so the point is probably moot.

>>> If the contents of the control file are tweaked before sending it
>>> through a BASE_BACKUP, it would cover more than just pg_basebackup.
>>> Switching the way the control file is sent with new contents in
>>> sendFileWithContent() rather than sendFile() would be one way, for
>>> instance..
>>
>> Good point, and that makes this even more compelling. If we include
>> pg_control into backup_label then there is no need to modify pg_control (as
>> above) -- we can just exclude it from the backup entirely. That will
>> certainly require some rejigging in recovery but seems worth it for backup
>> solutions that can't easily modify pg_control. The C-based solutions can do
>> this pretty easily but it is a pretty high bar for anyone else.
> 
> I have little idea about that, but I guess that you are referring to
> backrest here.

Sure, pgBackRest, but there are other backup solutions written in C. My 
point is really that we should not depend on backup solutions being able 
to manipulate C structs. It looks the the solution we are working 
towards would not require that.

Regards,
-David



Re: The danger of deleting backup_label

From
David Steele
Date:
On 10/12/23 10:19, David Steele wrote:
> On 10/11/23 18:10, Thomas Munro wrote:
> 
>> As Stephen mentioned[1], we could perhaps also complain if both backup
>> label and control file exist, and then hint that the user should
>> remove the *control file* (not the backup label!).  I had originally
>> suggested we would just overwrite the control file, but by explicitly
>> complaining about it we would also bring the matter to tool/script
>> authors' attention, ie that they shouldn't be backing that file up, or
>> should be removing it in a later step if they copy everything.  He
>> also mentions that there doesn't seem to be anything stopping us from
>> back-patching changes to the backup label contents if we go this way.
>> I don't have a strong opinion on that and we could leave the question
>> for later.
> 
> I'm worried about the possibility of back patching this unless the 
> solution comes out to be simpler than I think and that rarely comes to 
> pass. Surely throwing errors on something that is currently valid (i.e. 
> backup_label and pg_control both present).
> 
> But perhaps there is a simpler, acceptable solution we could back patch 
> (transparent to all parties except Postgres) and then a more advanced 
> solution we could go forward with.
> 
> I guess I had better get busy on this.

Attached is a very POC attempt at something along these lines that could 
be back patched. I stopped when I realized excluding pg_control from the 
backup is a necessity to make this work (else we still could end up with 
a torn copy of pg_control even if there is a good copy elsewhere). To 
enumerate the back patch issues as I see them:

1) We still need a copy of pg_control in order to get Postgres to start 
and that copy might be torn (pretty much where we are now). We can 
handle this easily in pg_basebackup but most backup software will not be 
able to do so. In my opinion teaching Postgres to start without 
pg_control is too big a change to possibly back patch.

2) We need to deal with backups made with a prior *minor* version that 
did not include pg_control in the backup_label. Doable, but...

3) We need to move backup_label to the end of the main pg_basebackup 
tar, which could cause unforeseen breakage for tools.

4) This patch is less efficient for backups taken from standby because 
it will overwrite pg_control on restart and force replay back to the 
original MinRecoveryPoint.

5) We still need a solution for exclusive backup (still valid in PG <= 
14). Doable but it would still have the weakness of 1.

All of this is fixable in HEAD, but seems incredibly dangerous to back 
patch. Even so, I have attached the patch in case somebody sees an 
opportunity that I do not.

Regards,
-David
Attachment

Re: The danger of deleting backup_label

From
Robert Haas
Date:
On Sat, Oct 14, 2023 at 11:33 AM David Steele <david@pgmasters.net> wrote:
> All of this is fixable in HEAD, but seems incredibly dangerous to back
> patch. Even so, I have attached the patch in case somebody sees an
> opportunity that I do not.

I really do not think we should be even thinking about back-patching
something like this. It's clearly not a bug fix, although I'm sure
that someone can try to characterize it that way, if they want to make
the well-worn argument that any behavior they don't like is a bug. But
that's a pretty lame argument. Usage errors on the part of users are
not bugs, even if we've coded the software in such a way as to make
those errors more likely.

I think what we ought to be talking about is whether a change like
this is a good idea even in master. I don't think it's a terrible
idea, but I'm also not sure that it's a good idea. The problem is that
if you're doing the right thing with your backup_label, then this is
unnecessary, and if you're doing the wrong thing, then why should you
do the right thing about this? I mean, admittedly you can't just
ignore a fatal error, but I think people will just run pg_resetwal,
which is even worse than starting from the wrong checkpoint. I feel
like in cases where a customer I'm working with has a bad backup,
their entire focus is on doing something to that backup to get a
running system back, whatever it takes. It's already too late at that
point to fix the backup procedure - they only have the backups they
have. You could hope people would do test restores before disaster
strikes, but people who are that prepared are probably running a real
backup tool and will never have this problem in the first place.

Perhaps that's all too pessimistic. I don't know. Certainly, other
people can have experiences that are different than mine. But I feel
like I struggle to think of a case where this would have prevented a
bad outcome, and that makes me wonder whether it's really a good idea
to complicate the system.

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



Re: The danger of deleting backup_label

From
David Steele
Date:
On 10/16/23 10:55, Robert Haas wrote:
> On Sat, Oct 14, 2023 at 11:33 AM David Steele <david@pgmasters.net> wrote:
>> All of this is fixable in HEAD, but seems incredibly dangerous to back
>> patch. Even so, I have attached the patch in case somebody sees an
>> opportunity that I do not.
> 
> I really do not think we should be even thinking about back-patching
> something like this. It's clearly not a bug fix, although I'm sure
> that someone can try to characterize it that way, if they want to make
> the well-worn argument that any behavior they don't like is a bug. But
> that's a pretty lame argument. Usage errors on the part of users are
> not bugs, even if we've coded the software in such a way as to make
> those errors more likely.

Hmmm, the reason to back patch this is that it would fix [1], which sure 
looks like a problem to me even if it is not a "bug". We can certainly 
require backup software to retry pg_control until the checksum is valid 
but that seems like a pretty big ask, even considering how complicated 
backup is.

I investigated this as a solution to [1] because it seemed like a better 
solution that what we have in [1]. But once I got into the weeds it was 
obvious that this wasn't going to be something we could back patch.

> I think what we ought to be talking about is whether a change like
> this is a good idea even in master. I don't think it's a terrible
> idea, but I'm also not sure that it's a good idea. The problem is that
> if you're doing the right thing with your backup_label, then this is
> unnecessary, and if you're doing the wrong thing, then why should you
> do the right thing about this? 

First and foremost, this solves the problem of pg_control being torn 
when read by the backup software. It can't be torn if it is not there.

There are also other advantages -- we can massage pg_control before 
writing it back out. This already happens, but before that happens there 
is a copy of the (prior) running pg_control there that can mess up the 
process.

> I mean, admittedly you can't just
> ignore a fatal error, but I think people will just run pg_resetwal,
> which is even worse than starting from the wrong checkpoint. 

If you start from the last checkpoint (which is what will generally be 
stored in pg_control) then the effect is pretty similar.

> I feel
> like in cases where a customer I'm working with has a bad backup,
> their entire focus is on doing something to that backup to get a
> running system back, whatever it takes. It's already too late at that
> point to fix the backup procedure - they only have the backups they
> have. You could hope people would do test restores before disaster
> strikes, but people who are that prepared are probably running a real
> backup tool and will never have this problem in the first place.

Right now the user can remove backup_label and get a "successful" 
restore and not realize that they have just corrupted their cluster. 
This is independent of the backup/restore tool doing all the right things.

My goal here is to narrow the options to try and make it so there is 
*one* valid procedure that will work. For this patch the idea is that 
they *must* start Postgres to get a valid pg_control from the 
backup_label. Any other action leads to a fatal error.

Note that the current patch is very WIP and does not actually do 
everything I'm talking about here. I was just trying to see if it could 
be used to solve the problem in [1]. It can't.

> Perhaps that's all too pessimistic. I don't know. Certainly, other
> people can have experiences that are different than mine. But I feel
> like I struggle to think of a case where this would have prevented a
> bad outcome, and that makes me wonder whether it's really a good idea
> to complicate the system.

I'm specifically addressing cases like those that came up (twice!) in 
[2]. This is the main place I see people stumbling these days. If even 
hackers can make this mistake then we should do better.

Regards,
-David

[1] 
https://www.postgresql.org/message-id/20221123014224.xisi44byq3cf5psi%40awork3.anarazel.de
[2] [1] 

https://www.postgresql.org/message-id/flat/CAM_vCudkSjr7NsNKSdjwtfAm9dbzepY6beZ5DP177POKy8%3D2aw%40mail.gmail.com#746e492bfcd2667635634f1477a61288



Re: The danger of deleting backup_label

From
Robert Haas
Date:
On Mon, Oct 16, 2023 at 11:45 AM David Steele <david@pgmasters.net> wrote:
> Hmmm, the reason to back patch this is that it would fix [1], which sure
> looks like a problem to me even if it is not a "bug". We can certainly
> require backup software to retry pg_control until the checksum is valid
> but that seems like a pretty big ask, even considering how complicated
> backup is.

That seems like a problem with pg_control not being written atomically
when the standby server is updating it during recovery, rather than a
problem with backup_label not being used at the start of recovery.
Unless I'm confused.

> If you start from the last checkpoint (which is what will generally be
> stored in pg_control) then the effect is pretty similar.

If the backup didn't span a checkpoint, then restoring from the one in
pg_control actually works fine. Not that I'm encouraging that. But if
you replay WAL from the control file, you at least get the last
checkpoint's worth of WAL; if you use pg_resetwal, you get nothing.

I don't really want to get hung up on this though. My main point here
is that I have trouble believing that an error after you've already
screwed up your backup helps much. I think what we need is to make it
less likely that you will screw up your backup in the first place.

> Right now the user can remove backup_label and get a "successful"
> restore and not realize that they have just corrupted their cluster.
> This is independent of the backup/restore tool doing all the right things.

I don't think it's independent of that at all.

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



Re: The danger of deleting backup_label

From
David Steele
Date:
On 10/16/23 12:25, Robert Haas wrote:
> On Mon, Oct 16, 2023 at 11:45 AM David Steele <david@pgmasters.net> wrote:
>> Hmmm, the reason to back patch this is that it would fix [1], which sure
>> looks like a problem to me even if it is not a "bug". We can certainly
>> require backup software to retry pg_control until the checksum is valid
>> but that seems like a pretty big ask, even considering how complicated
>> backup is.
> 
> That seems like a problem with pg_control not being written atomically
> when the standby server is updating it during recovery, rather than a
> problem with backup_label not being used at the start of recovery.
> Unless I'm confused.

You are not confused. But the fact that it practically can be read as 
torn at all on the standby is a function of how rapidly it is being 
written to update min recovery point. Writing it atomically via a temp 
file may affect performance in this area, but only during the backup, 
which may cause recovery to run more slowly during a backup.

I don't have proof of this because I don't have any hosts large enough 
to test the theory.

>> Right now the user can remove backup_label and get a "successful"
>> restore and not realize that they have just corrupted their cluster.
>> This is independent of the backup/restore tool doing all the right things.
> 
> I don't think it's independent of that at all.

I think it is. Imagine the user does backup/recovery using their 
favorite tool and everything is fine. But due to some misconfiguration 
or a problem in the WAL archive, they get this error:

FATAL:  could not locate required checkpoint record
2023-10-16 16:42:50.132 UTC HINT:  If you are restoring from a backup, 
touch "/home/dev/test/data/recovery.signal" or 
"/home/dev/test/data/standby.signal" and add required recovery options.
         If you are not restoring from a backup, try removing the file 
"/home/dev/test/data/backup_label".
         Be careful: removing "/home/dev/test/data/backup_label" will 
result in a corrupt cluster if restoring from a backup.

I did this by setting restore_command=false, but it could just as easily 
be the actual restore command that returns false due to a variety of 
reasons. The user has no idea what "could not locate required checkpoint 
record" means and if there is enough automation they may not even 
realize they just restored from a backup.

After some agonizing (we hope) they decide to delete backup_label and, 
wow, it just works! So now they merrily go on their way with a corrupted 
cluster. They also remember for the next time that deleting backup_label 
is definitely a good procedure.

The idea behind this patch is that deleting backup_label would produce a 
hard error because pg_control would be missing as well (if the backup 
software did its job). If both pg_control and backup_label are present 
(but pg_control has not been loaded with the contents of backup_label, 
i.e. it is the running copy from the backup cluster) we can also error.

It's not perfect, because they could backup (or restore) pg_control but 
not backup_label, but we are narrowing the cases where something can go 
wrong and they have silent corruption, especially if their 
backup/restore software follows the directions.

Regards,
-David



Re: The danger of deleting backup_label

From
Robert Haas
Date:
On Mon, Oct 16, 2023 at 1:00 PM David Steele <david@pgmasters.net> wrote:
> After some agonizing (we hope) they decide to delete backup_label and,
> wow, it just works! So now they merrily go on their way with a corrupted
> cluster. They also remember for the next time that deleting backup_label
> is definitely a good procedure.
>
> The idea behind this patch is that deleting backup_label would produce a
> hard error because pg_control would be missing as well (if the backup
> software did its job). If both pg_control and backup_label are present
> (but pg_control has not been loaded with the contents of backup_label,
> i.e. it is the running copy from the backup cluster) we can also error.

I mean, I think we're just going in circles, here. I did and do
understand, but I didn't and don't agree. You're hypothesizing a user
who is willing to do ONE thing that they shouldn't do during backup
restoration (namely, remove backup_label) but who won't be willing to
do a SECOND thing that they shouldn't do during backup restoration
(namely, run pg_resetwal). In my experience, users who are willing to
corrupt their database don't typically limit themselves to one bad
decision, and therefore I doubt that this proposal delivers enough
value to justify the complexity.

I understand that you feel differently, and that's fine, but I don't
think our disagreement here stems from me being confused. I just ...
don't agree.

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



Re: The danger of deleting backup_label

From
Michael Paquier
Date:
On Mon, Oct 16, 2023 at 12:25:59PM -0400, Robert Haas wrote:
> On Mon, Oct 16, 2023 at 11:45 AM David Steele <david@pgmasters.net> wrote:
> > If you start from the last checkpoint (which is what will generally be
> > stored in pg_control) then the effect is pretty similar.
>
> If the backup didn't span a checkpoint, then restoring from the one in
> pg_control actually works fine. Not that I'm encouraging that. But if
> you replay WAL from the control file, you at least get the last
> checkpoint's worth of WAL; if you use pg_resetwal, you get nothing.

There's no guarantee that the backend didn't spawn an extra checkpoint
while a base backup was taken, either, because we don't fail hard at
the end of the BASE_BACKUP code paths if the redo LSN has been updated
since the beginning of a BASE_BACKUP.  So that's really *never* do it
except if you like silent corruptions.

> I don't really want to get hung up on this though. My main point here
> is that I have trouble believing that an error after you've already
> screwed up your backup helps much. I think what we need is to make it
> less likely that you will screw up your backup in the first place.

Yeah..  Now what's the best user experience?  Is it better for a base
backup to fail and have a user retry?  Or is it better to have the
backend-side backup logic do what we think is safer?  The former
(likely with a REDO check or similar), will likely never work on large
instances, while users will likely always find ways to screw up base
backups taken by latter methods.  A third approach is to put more
careful checks at restore time, and the latter helps a lot here.
--
Michael

Attachment

Re: The danger of deleting backup_label

From
David Steele
Date:
On 10/16/23 15:06, Robert Haas wrote:
> On Mon, Oct 16, 2023 at 1:00 PM David Steele <david@pgmasters.net> wrote:
>> After some agonizing (we hope) they decide to delete backup_label and,
>> wow, it just works! So now they merrily go on their way with a corrupted
>> cluster. They also remember for the next time that deleting backup_label
>> is definitely a good procedure.
>>
>> The idea behind this patch is that deleting backup_label would produce a
>> hard error because pg_control would be missing as well (if the backup
>> software did its job). If both pg_control and backup_label are present
>> (but pg_control has not been loaded with the contents of backup_label,
>> i.e. it is the running copy from the backup cluster) we can also error.
> 
> I mean, I think we're just going in circles, here. I did and do
> understand, but I didn't and don't agree. You're hypothesizing a user
> who is willing to do ONE thing that they shouldn't do during backup
> restoration (namely, remove backup_label) but who won't be willing to
> do a SECOND thing that they shouldn't do during backup restoration
> (namely, run pg_resetwal). 

In my experience the first case is much more likely than the second. 
Your experience may vary.

Anyway, I think they are pretty different. Deleting backup label appears 
to give a perfectly valid restore. Running pg_resetwal is more clearly 
(I think) the nuclear solution.

> I understand that you feel differently, and that's fine, but I don't
> think our disagreement here stems from me being confused. I just ...
> don't agree.

Fair enough, we don't agree.

Regards,
-David



Re: The danger of deleting backup_label

From
Stephen Frost
Date:
Greetings,

* David Steele (david@pgmasters.net) wrote:
> On 10/16/23 15:06, Robert Haas wrote:
> > On Mon, Oct 16, 2023 at 1:00 PM David Steele <david@pgmasters.net> wrote:
> > > After some agonizing (we hope) they decide to delete backup_label and,
> > > wow, it just works! So now they merrily go on their way with a corrupted
> > > cluster. They also remember for the next time that deleting backup_label
> > > is definitely a good procedure.
> > >
> > > The idea behind this patch is that deleting backup_label would produce a
> > > hard error because pg_control would be missing as well (if the backup
> > > software did its job). If both pg_control and backup_label are present
> > > (but pg_control has not been loaded with the contents of backup_label,
> > > i.e. it is the running copy from the backup cluster) we can also error.
> >
> > I mean, I think we're just going in circles, here. I did and do
> > understand, but I didn't and don't agree. You're hypothesizing a user
> > who is willing to do ONE thing that they shouldn't do during backup
> > restoration (namely, remove backup_label) but who won't be willing to
> > do a SECOND thing that they shouldn't do during backup restoration
> > (namely, run pg_resetwal).
>
> In my experience the first case is much more likely than the second. Your
> experience may vary.

My experience (though perhaps not a surprise) mirrors David's.

> Anyway, I think they are pretty different. Deleting backup label appears to
> give a perfectly valid restore. Running pg_resetwal is more clearly (I
> think) the nuclear solution.

Right, and a delete of backup_label is just an 'rm' that folks may think
"oh, this is just some leftover thing that isn't actually needed".

OTOH, pg_resetwal has an online documentation page and a man page that's
very clear that it's only to be used as a last resort (perhaps we should
pull that into the --help output too..?).  It's also pretty clear that
pg_resetwal is actually changing things about the cluster while nuking
backup_label doesn't *seem* to be in that same category, even though we
all know it is because it's needed once recovery begins.

I'd also put out there that while people don't do restore testing
nearly as much as they should, they tend to at _least_ try to do it once
after taking their first backup and if that fails then they try to figure
out why and what they're not doing right.

Thanks,

Stephen

Attachment

Re: The danger of deleting backup_label

From
Robert Haas
Date:
On Tue, Oct 17, 2023 at 3:17 PM Stephen Frost <sfrost@snowman.net> wrote:
> I'd also put out there that while people don't do restore testing
> nearly as much as they should, they tend to at _least_ try to do it once
> after taking their first backup and if that fails then they try to figure
> out why and what they're not doing right.

Well, I agree with you on that point, but a lot of people only seem to
realize that after it's too late.

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



Re: The danger of deleting backup_label

From
David Steele
Date:
On 10/14/23 11:30, David Steele wrote:
> On 10/12/23 10:19, David Steele wrote:
>> On 10/11/23 18:10, Thomas Munro wrote:
>>
>>> As Stephen mentioned[1], we could perhaps also complain if both backup
>>> label and control file exist, and then hint that the user should
>>> remove the *control file* (not the backup label!).  I had originally
>>> suggested we would just overwrite the control file, but by explicitly
>>> complaining about it we would also bring the matter to tool/script
>>> authors' attention, ie that they shouldn't be backing that file up, or
>>> should be removing it in a later step if they copy everything.  He
>>> also mentions that there doesn't seem to be anything stopping us from
>>> back-patching changes to the backup label contents if we go this way.
>>> I don't have a strong opinion on that and we could leave the question
>>> for later.
>>
>> I'm worried about the possibility of back patching this unless the 
>> solution comes out to be simpler than I think and that rarely comes to 
>> pass. Surely throwing errors on something that is currently valid 
>> (i.e. backup_label and pg_control both present).
>>
>> But perhaps there is a simpler, acceptable solution we could back 
>> patch (transparent to all parties except Postgres) and then a more 
>> advanced solution we could go forward with.
>>
>> I guess I had better get busy on this.
> 
> Attached is a very POC attempt at something along these lines that could 
> be back patched. I stopped when I realized excluding pg_control from the 
> backup is a necessity to make this work (else we still could end up with 
> a torn copy of pg_control even if there is a good copy elsewhere). To 
> enumerate the back patch issues as I see them:

Given that the above can't be back patched, I'm thinking we don't need 
backup_label at all going forward. We just write the values we need for 
recovery into pg_control and return *that* from pg_backup_stop() and 
tell the user to store it with their backup. We already have "These 
files are vital to the backup working and must be written byte for byte 
without modification, which may require opening the file in binary 
mode." in the documentation so dealing with pg_control should not be a 
problem. pg_control also has a CRC so we will know if it gets munged.

It doesn't really matter where/how they store pg_control as long as it 
is written back into PGDATA before the cluster starts. If 
backupEndRequired, etc., are set appropriately then recovery will do the 
right thing when it starts, just as now if PG crashes after it has 
renamed backup_label but before recovery to consistency has completed.

We can still enforce the presence of recovery.signal by checking 
backupEndRequired if that's something we want but it seems like 
backupEndRequired would be enough. I'm fine either way.

Another thing we can do here is make backup from standby easier. The 
user won't need to worry about *when* pg_control is copied. We can just 
write the ideal min recovery point into pg_control.

Any informational data currently in backup_label can be returned as 
columns (like the start/end lsn is now).

This makes the patch much less invasive and while it definitely, 
absolutely cannot be back patched, it seems like a good way forward.

This is the direction I'm planning to work on patch-wise but I'd like to 
hear people's thoughts.

Regards,
-David



Re: The danger of deleting backup_label

From
Kyotaro Horiguchi
Date:
At Tue, 17 Oct 2023 16:16:42 -0400, David Steele <david@pgmasters.net> wrote in 
> Given that the above can't be back patched, I'm thinking we don't need
> backup_label at all going forward. We just write the values we need
> for recovery into pg_control and return *that* from pg_backup_stop()
> and tell the user to store it with their backup. We already have
> "These files are vital to the backup working and must be written byte
> for byte without modification, which may require opening the file in
> binary mode." in the documentation so dealing with pg_control should
> not be a problem. pg_control also has a CRC so we will know if it gets
> munged.

I'm somewhat perplexed regarding the objective of this thread.

This thread began with the intent of preventing users from removing
the backup_label from a backup. At the beginning, the proposal aimed
to achieve this by injecting an invalid value to pg_control file
located in the generated backup. However, this (and previous) proposal
seems to deviate from that initial objective. It now eliminates the
need to be concerned about the pg_control version that is coped into
the generated backup. However, if someone removes the backup_label
from a backup, the initial concerns could still surface.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: The danger of deleting backup_label

From
Robert Haas
Date:
On Tue, Oct 17, 2023 at 4:17 PM David Steele <david@pgmasters.net> wrote:
> Given that the above can't be back patched, I'm thinking we don't need
> backup_label at all going forward. We just write the values we need for
> recovery into pg_control and return *that* from pg_backup_stop() and
> tell the user to store it with their backup. We already have "These
> files are vital to the backup working and must be written byte for byte
> without modification, which may require opening the file in binary
> mode." in the documentation so dealing with pg_control should not be a
> problem. pg_control also has a CRC so we will know if it gets munged.

Yeah, I was thinking about this kind of idea, too. I think it might be
a good idea, although I'm not completely certain about that, either.

On the positive side, you can't remove backup_label in error if
backup_label is not a thing. You certainly can't remove the control
file. You can, however, use the original control file instead of the
one that you were supposed to use. However, that is not really any
different from failing to write the backup_label into the backup
directory, which you can already do today. Also, it adds very little
net complexity to the low-level backup procedure. Instead of needing
to write the backup_label into the backup directory, you write the
control file -- but that's instead, not in addition. So overall it
seems like the complexity is similar to what we have today but one
possible mistake is eliminated.

Also on the positive side, I suspect we could remove a decent amount
of code for dealing with backup_label files. We wouldn't have to read
them any more (and the code to do that is pretty rough-and-ready) and
we wouldn't have to do different things based on whether the
backup_label exists or not. The logic in xlog*.c is extremely
complicated, and everything that we can do to reduce the number of
cases that need to be considered is not just good, but great.

But there are also some negatives.

First, anything that is stored in the backup_label but not the control
file has to (a) move into the control file, (b) be stored someplace
else, or (c) be eliminated as a concept. We're likely to get
complaints about (a), especially if the data in question is anything
big. Any proposal to do (b) risks undermining the whole theory under
which this is a good proposal, namely that removing backup_label gives
us one less thing to worry about. So that brings us to (c).
Personally, I would lose very little sleep if the LABEL field died and
never came back, and I wouldn't miss START TIME and STOP TIME either,
but somebody else might well feel differently. I don't think it's
trivial to get rid of BACKUP METHOD, as there unfortunately seems to
be code that depends on knowing the difference between BACKUP FROM:
streamed and BACKUP FROM: pg_rewind. I suspect that BACKUP FROM:
primary/standby might have the same issue, but I'm not sure. STOP
TIMELINE could be a problem too. I think that if somebody could do
some rejiggering to eliminate some of the differences between the
cases here, that could be really good general cleanup irrespective of
what we decide about this proposal, and moving some things in to
pg_control is probably reasonable too. For instance, it would seem
crazy to me to argue that storing the backup end location in the
control file is OK, but storing the backup end TLI there would not be
OK. But the point here is that there's probably a good deal of careful
thinking that would need to be done here about exactly where all of
the stuff that currently exists in the backup_label file but not in
pg_control needs to end up.

Second, right now, the stuff that we return at the end of a backup is
all text data. With this proposal, it becomes binary data. I entirely
realize that people should only be doing these kinds of backups using
automated tools that that those automated tools should be perfectly
capable of handling binary data without garbling anything. But that's
about as realistic as supposing that people won't instantly remove the
backup_label file the moment it seems like it will solve some problem,
even when the directions clearly state that this should only be done
in some other situation that is not the one the user is facing. It
just occurred to me that one thing we could do to improve the user
experience here is offer some kind of command-line utility to assist
with taking a low-level backup. This could be done even if we don't
proceed with this proposal e.g.

pg_llbackup -d $CONNTR --backup-label=PATH --tablespace-map=PATH
--copy-data-directory=SHELLCOMMAND

I don't know for sure how much that would help, but I wonder if it
might actually help quite a bit, because right now people do things
like use psql in a shell script to try to juggle a database connection
and then in some other part of the shell script do the data copying.
But it is quite easy to screw up the error handling or the psql
session lifetime or something like that, and this would maybe provide
a nicer interface. Details likely need a good deal of kibitizing.

There might be other problems, too. This is just what occurs to me off
the top of my head. But I think it's an interesting angle to explore
further.

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



Re: The danger of deleting backup_label

From
David Steele
Date:
On 10/17/23 22:13, Kyotaro Horiguchi wrote:
> At Tue, 17 Oct 2023 16:16:42 -0400, David Steele <david@pgmasters.net> wrote in
>> Given that the above can't be back patched, I'm thinking we don't need
>> backup_label at all going forward. We just write the values we need
>> for recovery into pg_control and return *that* from pg_backup_stop()
>> and tell the user to store it with their backup. We already have
>> "These files are vital to the backup working and must be written byte
>> for byte without modification, which may require opening the file in
>> binary mode." in the documentation so dealing with pg_control should
>> not be a problem. pg_control also has a CRC so we will know if it gets
>> munged.
> 
> I'm somewhat perplexed regarding the objective of this thread.
> 
> This thread began with the intent of preventing users from removing
> the backup_label from a backup. At the beginning, the proposal aimed
> to achieve this by injecting an invalid value to pg_control file
> located in the generated backup. However, this (and previous) proposal
> seems to deviate from that initial objective. It now eliminates the
> need to be concerned about the pg_control version that is coped into
> the generated backup. However, if someone removes the backup_label
> from a backup, the initial concerns could still surface.

Yeah, the discussion has moved around quite a bit, but the goal remains 
the same, to make Postgres error when it does not have the information 
it needs to proceed with recovery. Right now if you delete backup_label 
recovery appears to complete successfully, silently corrupting the database.

In the proposal as it stands now there would be no backup_label at all, 
so no danger of removing it.

We have also gotten a bit sidetracked by our hope to use this proposal 
to address torn reads of pg_control during the backup, at least in HEAD.

Regards,
-David



Re: The danger of deleting backup_label

From
David Steele
Date:
On 10/18/23 08:39, Robert Haas wrote:
> On Tue, Oct 17, 2023 at 4:17 PM David Steele <david@pgmasters.net> wrote:
>> Given that the above can't be back patched, I'm thinking we don't need
>> backup_label at all going forward. We just write the values we need for
>> recovery into pg_control and return *that* from pg_backup_stop() and
>> tell the user to store it with their backup. We already have "These
>> files are vital to the backup working and must be written byte for byte
>> without modification, which may require opening the file in binary
>> mode." in the documentation so dealing with pg_control should not be a
>> problem. pg_control also has a CRC so we will know if it gets munged.
> 
> Yeah, I was thinking about this kind of idea, too. I think it might be
> a good idea, although I'm not completely certain about that, either.

<snip>

> First, anything that is stored in the backup_label but not the control
> file has to (a) move into the control file, 

I'd rather avoid this.

> (b) be stored someplace
> else, 

I don't think the additional fields *need* to be stored anywhere at all, 
at least not by us. We can provide them as output from pg_backup_stop() 
and the caller can do as they please. None of those fields are part of 
the restore process.

> or (c) be eliminated as a concept. 

I think we should keep them as above since I don't believe they hurt 
anything.

> We're likely to get
> complaints about (a), especially if the data in question is anything
> big. Any proposal to do (b) risks undermining the whole theory under
> which this is a good proposal, namely that removing backup_label gives
> us one less thing to worry about. So that brings us to (c).
> Personally, I would lose very little sleep if the LABEL field died and
> never came back, and I wouldn't miss START TIME and STOP TIME either,
> but somebody else might well feel differently. I don't think it's
> trivial to get rid of BACKUP METHOD, as there unfortunately seems to
> be code that depends on knowing the difference between BACKUP FROM:
> streamed and BACKUP FROM: pg_rewind. I suspect that BACKUP FROM:
> primary/standby might have the same issue, but I'm not sure. 

BACKUP FROM: primary/standby we can definitely handle, and BACKUP 
METHOD: pg_rewind just means backupEndRequired is false. That will need 
to be written by pg_rewind (instead of writing backup_label).

> STOP
> TIMELINE could be a problem too. I think that if somebody could do
> some rejiggering to eliminate some of the differences between the
> cases here, that could be really good general cleanup irrespective of
> what we decide about this proposal, and moving some things in to
> pg_control is probably reasonable too. For instance, it would seem
> crazy to me to argue that storing the backup end location in the
> control file is OK, but storing the backup end TLI there would not be
> OK. 

We have a place in pg_control for the end TLI (minRecoveryPointTLI). 
That's only valid for backup from standby since a primary backup can 
never change timelines.

> But the point here is that there's probably a good deal of careful
> thinking that would need to be done here about exactly where all of
> the stuff that currently exists in the backup_label file but not in
> pg_control needs to end up.

Agreed.

> Second, right now, the stuff that we return at the end of a backup is
> all text data. With this proposal, it becomes binary data. I entirely
> realize that people should only be doing these kinds of backups using
> automated tools that that those automated tools should be perfectly
> capable of handling binary data without garbling anything. But that's
> about as realistic as supposing that people won't instantly remove the
> backup_label file the moment it seems like it will solve some problem,
> even when the directions clearly state that this should only be done
> in some other situation that is not the one the user is facing. 

Well, we do specify that backup_label and tablespace_map should be saved 
byte for byte. But We've already seen users mess this up in the past and 
add \r characters that made the files unreadable.

If it can be done wrong, it will be done wrong by somebody.

> It
> just occurred to me that one thing we could do to improve the user
> experience here is offer some kind of command-line utility to assist
> with taking a low-level backup. This could be done even if we don't
> proceed with this proposal e.g.
> 
> pg_llbackup -d $CONNTR --backup-label=PATH --tablespace-map=PATH
> --copy-data-directory=SHELLCOMMAND
> 
> I don't know for sure how much that would help, but I wonder if it
> might actually help quite a bit, because right now people do things
> like use psql in a shell script to try to juggle a database connection
> and then in some other part of the shell script do the data copying.
> But it is quite easy to screw up the error handling or the psql
> session lifetime or something like that, and this would maybe provide
> a nicer interface. 

I think in most cases where this would be useful the user should just be 
using pg_basebackup. If the backup is trying to use snapshots, then 
backup_label needs to be stored outside the snapshot and we won't be 
able to easily help.

> There might be other problems, too. This is just what occurs to me off
> the top of my head. But I think it's an interesting angle to explore
> further.

There may definitely be other problems and I'm pretty sure there will 
be. My feeling is that they will be surmountable, but I won't know for 
sure until I finish the patch.

But I also feel it's a good idea to explore further. I'll work on the 
patch and should have something to share soon.

Regards,
-David



Re: The danger of deleting backup_label

From
"David G. Johnston"
Date:
On Wednesday, October 18, 2023, David Steele <david@pgmasters.net> wrote:
On 10/18/23 08:39, Robert Haas wrote:
On Tue, Oct 17, 2023 at 4:17 PM David Steele <david@pgmasters.net> wrote:
Given that the above can't be back patched, I'm thinking we don't need
backup_label at all going forward. We just write the values we need for
recovery into pg_control and return *that* from pg_backup_stop() and
tell the user to store it with their backup. We already have "These
files are vital to the backup working and must be written byte for byte
without modification, which may require opening the file in binary
mode." in the documentation so dealing with pg_control should not be a
problem. pg_control also has a CRC so we will know if it gets munged.

Yeah, I was thinking about this kind of idea, too. I think it might be
a good idea, although I'm not completely certain about that, either.

<snip>

First, anything that is stored in the backup_label but not the control
file has to (a) move into the control file,

I'd rather avoid this.


If we are OK outputting custom pg_control file content from pg_backup_end to govern this then I’d probably just include “tablespace_map_required=true|false” and “backup_label_required=true” lines in it and leave everything else mostly the same, including the name.  In order for a server with those lines in its pg_control to boot it requires that a signal file be present along with any of the named files where *_required is true.  Upon successful completion those lines are removed from pg_control.

It seem unnecessary to move any and all relevant content into pg_control; just a flag to ensure that those files that are needed a present in the backup directory and whatever validation of those files is needed to ensure they provide sufficient data.

If the user removes those files without a backup the server is not going to start unless they make further obvious attempts to circumvent the design. Manually editing pg_comtrol being obvious circumventing.

This would seem to be a compatible change.  If you fail to use the pg_control from pg_backup_stop you don’t get the safeguard but everything still works.  Do we really believe we need to break/force-upgrade tooling to use this new procedure?  Depending on the answer to the torn pg_comtrol file problem which may indeed warrant such breakage.

David J.

Re: The danger of deleting backup_label

From
Robert Haas
Date:
On Wed, Oct 18, 2023 at 7:15 PM David Steele <david@pgmasters.net> wrote:
> > (b) be stored someplace
> > else,
>
> I don't think the additional fields *need* to be stored anywhere at all,
> at least not by us. We can provide them as output from pg_backup_stop()
> and the caller can do as they please. None of those fields are part of
> the restore process.

Not sure which fields we're talking about here. I agree that if
they're not really needed, we can return them and the user can keep
them or discard them as they wish. But in that case you might also ask
why bother even returning them.

> > pg_llbackup -d $CONNTR --backup-label=PATH --tablespace-map=PATH
> > --copy-data-directory=SHELLCOMMAND
> >
> I think in most cases where this would be useful the user should just be
> using pg_basebackup. If the backup is trying to use snapshots, then
> backup_label needs to be stored outside the snapshot and we won't be
> able to easily help.

Right, the idea of the above was that you would specify paths for the
backup label and the tablespace map that were outside of the snapshot
directory in that kind of case. But you couldn't screw up the line
endings or whatever because pg_llbackup would take care of that aspect
of it for you.

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



Re: The danger of deleting backup_label

From
David Steele
Date:
On 10/19/23 10:24, Robert Haas wrote:
> On Wed, Oct 18, 2023 at 7:15 PM David Steele <david@pgmasters.net> wrote:
>>> (b) be stored someplace
>>> else,
>>
>> I don't think the additional fields *need* to be stored anywhere at all,
>> at least not by us. We can provide them as output from pg_backup_stop()
>> and the caller can do as they please. None of those fields are part of
>> the restore process.
> 
> Not sure which fields we're talking about here. I agree that if
> they're not really needed, we can return them and the user can keep
> them or discard them as they wish. But in that case you might also ask
> why bother even returning them.

I'm specifically talking about START TIME and LABEL. They are currently 
stored in backup_label but not used for recovery. START TIMELINE is also 
not used, except as a cross check against START WAL LOCATION.

I'd still like to see most or all of these fields exposed through 
pg_backup_stop(). The user can choose to store them or not, but none of 
them will be required for recovery.

>>> pg_llbackup -d $CONNTR --backup-label=PATH --tablespace-map=PATH
>>> --copy-data-directory=SHELLCOMMAND
>>>
>> I think in most cases where this would be useful the user should just be
>> using pg_basebackup. If the backup is trying to use snapshots, then
>> backup_label needs to be stored outside the snapshot and we won't be
>> able to easily help.
> 
> Right, the idea of the above was that you would specify paths for the
> backup label and the tablespace map that were outside of the snapshot
> directory in that kind of case. But you couldn't screw up the line
> endings or whatever because pg_llbackup would take care of that aspect
> of it for you.

What I meant here (but said badly) is that in the case of snapshot 
backups, the backup_label and tablespace_map will likely need to be 
stored somewhere off the server since they can't be part of the 
snapshot, perhaps in a key store. In that case the backup software would 
still need to read the files from wherever we stored then and correctly 
handle them when storing elsewhere. If you were moving the files to say, 
S3, a similar thing needs to happen. In general, I think a locally 
mounted filesystem is very unlikely to be the final destination for 
these files, and if it is then probably pg_basebackup is your friend.

Regards,
-David




Re: The danger of deleting backup_label

From
Robert Haas
Date:
On Thu, Oct 19, 2023 at 10:43 AM David Steele <david@pgmasters.net> wrote:
> What I meant here (but said badly) is that in the case of snapshot
> backups, the backup_label and tablespace_map will likely need to be
> stored somewhere off the server since they can't be part of the
> snapshot, perhaps in a key store. In that case the backup software would
> still need to read the files from wherever we stored then and correctly
> handle them when storing elsewhere. If you were moving the files to say,
> S3, a similar thing needs to happen. In general, I think a locally
> mounted filesystem is very unlikely to be the final destination for
> these files, and if it is then probably pg_basebackup is your friend.

I mean, writing those tiny little files locally and then uploading
them should be fine in a case like that. It still reduces the surface
for mistakes. And you could also have --backup-label='| whatever' or
something if you wanted. The point is that right now we're asking
people to pull this information out of a query result, and that means
people are trying to do it by calling out to psql, and that is a GREAT
way to screw up the escaping or the newlines or whatever. I don't
think the mistakes people are making here are being made by people
using Perl and DBD::Pg, or Python and psycopg2, or C and libpq.
They're being made by people who are trying to shell script their way
through it, which entails using psql, which makes screwing it up a
breeze.

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



Re: The danger of deleting backup_label

From
"David G. Johnston"
Date:
On Thursday, October 19, 2023, David Steele <david@pgmasters.net> wrote:
On 10/19/23 10:24, Robert Haas wrote:
On Wed, Oct 18, 2023 at 7:15 PM David Steele <david@pgmasters.net> wrote:

pg_llbackup -d $CONNTR --backup-label=PATH --tablespace-map=PATH
--copy-data-directory=SHELLCOMMAND

I think in most cases where this would be useful the user should just be
using pg_basebackup. If the backup is trying to use snapshots, then
backup_label needs to be stored outside the snapshot and we won't be
able to easily help.

Right, the idea of the above was that you would specify paths for the
backup label and the tablespace map that were outside of the snapshot
directory in that kind of case. But you couldn't screw up the line
endings or whatever because pg_llbackup would take care of that aspect
of it for you.

What I meant here (but said badly) is that in the case of snapshot backups, the backup_label and tablespace_map will likely need to be stored somewhere off the server since they can't be part of the snapshot, perhaps in a key store. In that case the backup software would still need to read the files from wherever we stored then and correctly handle them when storing elsewhere. If you were moving the files to say, S3, a similar thing needs to happen. In general, I think a locally mounted filesystem is very unlikely to be the final destination for these files, and if it is then probably pg_basebackup is your friend.


We are choosing to not take responsibility if the procedure used by the dba is one that takes a full live backup of the data directory as-is and then brings that backup back into production without making any changes to it.  That restored copy will be bootable but quite probably corrupted.  That is on them as we have no way to make it non-bootable without risking source database being unable to be restarted automatically upon a crash.  In short, a snapshot methodology is beyond what we are willing to provide protections for.

What I do kinda gather from the above is we should be providing a pg_baserestore application if we want to give our users fewer reasons to write their own tooling against the API and/or want to add more complexity to pg_basebackup to handle various needs that need corresponding recovery needs that we should implement as code instead of documentation.

David J.


Re: The danger of deleting backup_label

From
David Steele
Date:
On 10/19/23 10:56, Robert Haas wrote:
> On Thu, Oct 19, 2023 at 10:43 AM David Steele <david@pgmasters.net> wrote:
>> What I meant here (but said badly) is that in the case of snapshot
>> backups, the backup_label and tablespace_map will likely need to be
>> stored somewhere off the server since they can't be part of the
>> snapshot, perhaps in a key store. In that case the backup software would
>> still need to read the files from wherever we stored then and correctly
>> handle them when storing elsewhere. If you were moving the files to say,
>> S3, a similar thing needs to happen. In general, I think a locally
>> mounted filesystem is very unlikely to be the final destination for
>> these files, and if it is then probably pg_basebackup is your friend.
> 
> I mean, writing those tiny little files locally and then uploading
> them should be fine in a case like that. It still reduces the surface
> for mistakes. And you could also have --backup-label='| whatever' or
> something if you wanted. The point is that right now we're asking
> people to pull this information out of a query result, and that means
> people are trying to do it by calling out to psql, and that is a GREAT
> way to screw up the escaping or the newlines or whatever. I don't
> think the mistakes people are making here are being made by people
> using Perl and DBD::Pg, or Python and psycopg2, or C and libpq.
> They're being made by people who are trying to shell script their way
> through it, which entails using psql, which makes screwing it up a
> breeze.

OK, I see what you mean and I agree. Better documentation might be the 
answer here, but I doubt that psql is a good tool for starting/stopping 
backup and not sure we want to encourage it.

Regards,
-David