Re: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?

On 7/26/22 07:59, Bharath Rupireddy wrote:
> On Tue, Jul 26, 2022 at 5:22 PM David Steele <david@pgmasters.net> wrote:
>>
>> On 7/25/22 22:49, Kyotaro Horiguchi wrote:
>>> At Mon, 25 Jul 2022 14:21:38 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.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?

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.

Regards,
-David