Message-ID: <c4adaf22-b3f4-4902-acd5-53ec07e88c3c@pgmasters.net>
Date: Mon, 20 Nov 2023 15:41:38 -0400
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add recovery to pg_control and remove backup_label
Content-Language: en-US
To: Robert Haas <robertmhaas@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>,
 Postgres hackers <pgsql-hackers@lists.postgresql.org>
References: <46c300b9-96a1-449c-8ecd-9003bd02cb6b@pgmasters.net>
 <ZUnzS_yCM0lPXMat@paquier.xyz> <ZU2zbPwGwy5ammRm@paquier.xyz>
 <f4762790-efc4-4dd1-a3d0-02d183d9466c@pgmasters.net>
 <ZVGn6PY0LQ7RbDVL@paquier.xyz>
 <77ad92e7-3e37-40e2-8e3b-df3cf0108aa8@pgmasters.net>
 <ZVQfgWvoGDWm37J_@paquier.xyz>
 <fb5a0e35-0ac0-4265-b0fe-135638adecd0@pgmasters.net>
 <ZVVcZ2HooBnhld_z@paquier.xyz>
 <a1d77886-97ac-4ec3-8f5d-7f0d95cbdc1e@pgmasters.net>
 <ZVqzRSbi2feDhmf7@paquier.xyz>
 <CA+TgmoZqyarS2ud8PXywh_+r6tsss4_D3=F+8PVSaa2MO+HPkA@mail.gmail.com>
 <38b212b7-b8c0-46ea-940b-f27ec3a24deb@pgmasters.net>
 <CA+TgmoYo1Gu5+0xy7-ivmjsdDfCPDPyyV+yL66XjMeOwTCEAFA@mail.gmail.com>
From: David Steele <david@pgmasters.net>
In-Reply-To: 
 <CA+TgmoYo1Gu5+0xy7-ivmjsdDfCPDPyyV+yL66XjMeOwTCEAFA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/c4adaf22-b3f4-4902-acd5-53ec07e88c3c%40pgmasters.net>
Precedence: bulk

On 11/20/23 14:44, Robert Haas wrote:
> On Mon, Nov 20, 2023 at 12:54 PM David Steele <david@pgmasters.net> wrote:
>> Another thing we could do is explicitly error if we see backup_label in
>> PGDATA during recovery. That's just a few lines of code so would not be
>> a big deal to maintain. This error would only be visible on restore, so
>> it presumes that backup software is being tested.
> 
> I think that if we do decide to adopt this proposal, that would be a
> smart precaution.

I'd be OK with it -- what do you think, Michael? Would this be enough 
that we would not need to rename the functions, or should we just go 
with the rename?

>> A little hard to add to the tests, I think, since they are purely
>> informational, i.e. not pushed up by the parser. Maybe we could just
>> grep for the fields?
> 
> Hmm. Or should they be pushed up by the parser?

We could do that. I started on that road, but it's a lot of code for 
fields that aren't used. I think it would be better if the parser also 
loaded a data structure that represented the manifest. Seems to me 
there's a lot of duplicated code between pg_verifybackup and 
pg_combinebackup the way it is now.

>> I think these fields would be handled the same as the rest of the fields
>> in backup_label: returned from pg_backup_stop() and also stored in
>> backup_manifest. Third-party software can do as they like with them and
>> pg_combinebackup can just read from backup_manifest.
> 
> I think that would be a bad plan, because this is critical
> information, and a backup manifest is not a thing that you're required
> to have. It's not a natural fit at all. We don't want to create a
> situation where if you nuke the backup_manifest then the server
> forgets that what it has is an incremental backup rather than a usable
> data directory.

I can't see why a backup would continue to be valid without a manifest 
-- that's not very standard for backup software. If you have the 
critical info in backup_label, you can't afford to lose that, so why 
should backup_manifest be any different?

Regards,
-David