On Tue, Jul 26, 2022 at 5:50 PM David Steele <david@pgmasters.net> wrote:
>
> >> I would prefer to have all the components of backup_label stored
> >> separately and then generate backup_label from them in pg_backup_stop().
> >
> > +1, because pg_backup_stop is the one that's returning backup_label
> > contents, so it does make sense for it to prepare it once and for all
> > and return.
> >
> >> For PG16 I am planning to add some fields to backup_label that are not
> >> known when pg_backup_start() is called, e.g. min recovery time.
> >
> > Can you please point to your patch that does above?
>
> Currently it is a plan, not a patch. So there is nothing to show yet.
>
> > Yes, right now, backup_label or tablespace_map contents are being
> > filled in by pg_backup_start and are never changed again. But if your
> > above proposal is for fixing some issue, then it would make sense for
> > us to carry all the info in a structure to pg_backup_stop and then let
> > it prepare the backup_label and tablespace_map contents.
>
> I think this makes sense even if I don't get these changes into PG16.
>
> > If the approach is okay for the hackers, I would like to spend time on it.
>
> +1 from me.
Here comes the v1 patch. This patch tries to refactor backup related
code, advantages of doing so are following:
1) backup state is more structured now - all in a single structure,
callers can create backup_label contents whenever required, either
during the pg_backup_start or the pg_backup_stop or in between.
2) no parsing of backup_label file lines now in pg_backup_stop, no
error checking for invalid parsing.
3) backup_label and history file contents have most of the things in
common, they can now be created within a single function.
4) makes backup related code extensible and readable.
One downside is that it creates a lot of diff with previous versions.
Please review.
--
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/
