Message-ID: <9dc67363-5200-7395-3ddb-b50171800bc6@pgmasters.net>
Date: Mon, 16 Oct 2023 11:45:07 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.15.1
Subject: Re: The danger of deleting backup_label
Content-Language: en-US
To: Robert Haas <robertmhaas@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>,
 Michael Paquier <michael@paquier.xyz>,
 Pg Hackers <pgsql-hackers@postgresql.org>, Stephen Frost <sfrost@snowman.net>
References: <1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net>
 <ZRY2vdlKJqhuZqGs@paquier.xyz>
 <0e088834-aa39-ed09-3ec7-31c13c629a20@pgmasters.net>
 <CA+hUKGLok_wrosp8Q6DA+mkhtWyQSEwPSKP=2p5bnLZC=FGpaQ@mail.gmail.com>
 <65825be1-e79a-46f4-9d9f-4ff95a10e378@pgmasters.net>
 <e05f20f9-6f91-9a70-efab-9a2ae472e65d@pgmasters.net>
 <CA+TgmoZkdrWyd7KiPFHaJBg+tjM3UFrqOBK1EtG3NtVs97-7Xw@mail.gmail.com>
From: David Steele <david@pgmasters.net>
In-Reply-To: <CA+TgmoZkdrWyd7KiPFHaJBg+tjM3UFrqOBK1EtG3NtVs97-7Xw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/9dc67363-5200-7395-3ddb-b50171800bc6%40pgmasters.net>
Precedence: bulk

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