On 7/25/22 22:49, Kyotaro Horiguchi wrote:
At Mon, 25 Jul 2022 14:21:38 +0530, Bharath Rupireddy
<bharath.rupireddyforpostg...@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
