On 7/26/22 07:59, Bharath Rupireddy wrote:
On Tue, Jul 26, 2022 at 5:22 PM David Steele <da...@pgmasters.net> wrote:
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().
+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
