On Thu, Aug 8, 2019 at 8:37 PM Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> wrote: > + if (!XLogRecPtrIsInvalid(previous_lsn)) > + appendStringInfo(labelfile, "PREVIOUS WAL LOCATION: %X/%X\n", > + (uint32) (previous_lsn >> 32), (uint32) previous_lsn); > > May be we should rename to something like: > "INCREMENTAL BACKUP START WAL LOCATION" or simply "INCREMENTAL BACKUP START LOCATION" > to make it more intuitive?
So, I think that you are right that PREVIOUS WAL LOCATION might not be entirely clear, but at least in my view, INCREMENTAL BACKUP START WAL LOCATION is definitely not clear. This backup is an incremental backup, and it has a start WAL location, so you'd end up with START WAL LOCATION and INCREMENTAL BACKUP START WAL LOCATION and those sound like they ought to both be the same thing, but they're not. Perhaps something like REFERENCE WAL LOCATION or REFERENCE WAL LOCATION FOR INCREMENTAL BACKUP would be clearer.
Agree, how about INCREMENTAL BACKUP REFERENCE WAL LOCATION ?
> File header structure is defined in both the files basebackup.c and > pg_combinebackup.c. I think it is better to move this to replication/basebackup.h.
Or some other header, but yeah, definitely don't duplicate the struct definition (or any other kind of definition).
Thanks.
> IMHO, while labels are not advisable in general, it may be better to use a label > here rather than a while(1) loop, so that we can move to the label in case we > want to retry once. I think here it opens doors for future bugs if someone > happens to add code here, ending up adding some condition and then the > break becomes conditional. That will leave us in an infinite loop.
I'm not sure which style is better here, but I don't really buy this argument.