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.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center