Message-ID: <0e088834-aa39-ed09-3ec7-31c13c629a20@pgmasters.net>
Date: Tue, 10 Oct 2023 17:06:45 -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
To: Michael Paquier <michael@paquier.xyz>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
References: <1330cb48-4e47-03ca-f2fb-b144b49514d8@pgmasters.net>
 <ZRY2vdlKJqhuZqGs@paquier.xyz>
Content-Language: en-US
From: David Steele <david@pgmasters.net>
In-Reply-To: <ZRY2vdlKJqhuZqGs@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/0e088834-aa39-ed09-3ec7-31c13c629a20%40pgmasters.net>
Precedence: bulk

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