Re: Add recovery to pg_control and remove backup_label

On 11/20/23 14:44, Robert Haas wrote:
> On Mon, Nov 20, 2023 at 12:54 PM David Steele <david(at)pgmasters(dot)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