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

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().

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.

Regards,
-David