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

On Thu, Jul 21, 2022 at 2:33 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Wed, 20 Jul 2022 17:09:09 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> > Hi,
> >
> > After the commit [1], is it correct to say errmsg("invalid data in file
> > \"%s\"", BACKUP_LABEL_FILE))); in do_pg_backup_stop() when we hold the
> > contents in backend global memory, not actually reading from backup_label
> > file? However, it is correct to say that in read_backup_label.
> >
> > IMO, we can either say "invalid backup_label contents found" or we can be
> > more descriptive and say "invalid "START WAL LOCATION" line found in
> > backup_label content" and "invalid "BACKUP FROM" line found in
> > backup_label content" and so on.
> >
> > Thoughts?
>
> Previously there the case the "char *labelfile" is loaded from a file,
> but currently it is alwasy a string build on the process. In that
> sense, nowadays it is a kind of internal error, which I think is not
> supposed to be exposed to users.
>
> So I think we can leave the code alone to avoid back-patching
> obstacles. But if we decided to change the code around, I'd like to
> change the string into a C struct, so that we don't need to parse it.

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?

Regards,
Bharath Rupireddy.