From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
---|---|
To: | David Steele <david(at)pgmasters(dot)net> |
Cc: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?) |
Date: | 2022-08-08 13:50:31 |
Message-ID: | CALj2ACU6hMCfxAw-rraMvp0oaSLnweQ0H6QoK3r7u9+sVA+qwg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sat, Jul 30, 2022 at 5:37 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Tue, Jul 26, 2022 at 5:50 PM David Steele <david(at)pgmasters(dot)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.
I added this to current CF - https://commitfest.postgresql.org/39/3808/
--
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
From | Date | Subject | |
---|---|---|---|
Next Message | Andrew Dunstan | 2022-08-08 13:59:10 | Re: bug on log generation ? |
Previous Message | Bharath Rupireddy | 2022-08-08 13:43:25 | Re: An attempt to avoid locally-committed-but-not-replicated-to-standby-transactions in synchronous replication |