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: | Re: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop? |
Date: | 2022-07-26 11:59:29 |
Message-ID: | CALj2ACWN69xLwzTC+Nca4dt9HmRP7vkRK9HPK0gNKuU5hfH+Yg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Jul 26, 2022 at 5:22 PM David Steele <david(at)pgmasters(dot)net> wrote:
>
> On 7/25/22 22:49, Kyotaro Horiguchi wrote:
> > At Mon, 25 Jul 2022 14:21:38 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> >> Hm. I think we must take this opportunity to clean it up. You are
> >> right, we don't need to parse the label file contents (just like we
> >> used to do previously after reading it from the file) in
> >> do_pg_backup_stop(), instead we can just pass a structure. Also,
> >> do_pg_backup_stop() isn't modifying any labelfile contents, but using
> >> startxlogfilename, startpoint and backupfrom from the labelfile
> >> contents. I think this information can easily be passed as a single
> >> structure. In fact, I might think a bit more here and wrap label_file,
> >> tblspc_map_file to a single structure something like below and pass it
> >> across the functions.
> >>
> >> typedef struct BackupState
> >> {
> >> StringInfo label_file;
> >> StringInfo tblspc_map_file;
> >> char startxlogfilename[MAXFNAMELEN];
> >> XLogRecPtr startpoint;
> >> char backupfrom[20];
> >> } BackupState;
> >>
> >> This way, the code is more readable, structured and we can remove 2
> >> sscanf() calls, 2 "invalid data in file" errors, 1 strchr() call, 1
> >> strstr() call. Only thing is that it creates code diff from the
> >> previous PG versions which is fine IMO. If okay, I'm happy to prepare
> >> a patch.
> >>
> >> Thoughts?
> >
> > It is more or less what was in my mind, but it seems that we don't
> > need StringInfo there, or should avoid it to signal the strings are
> > not editable.
>
> 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?
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.
If the approach is okay for the hackers, I would like to spend time on it.
Regards,
Bharath Rupireddy.
From | Date | Subject | |
---|---|---|---|
Next Message | Dave Cramer | 2022-07-26 12:11:04 | Re: Proposal to provide the facility to set binary format output for specific OID's per session |
Previous Message | Frédéric Yhuel | 2022-07-26 11:58:17 | Re: Allow parallel plan for referential integrity checks? |