Hi Robert, On Fri, Aug 9, 2019 at 6:40 PM Robert Haas <robertmh...@gmail.com> wrote:
> On Thu, Aug 8, 2019 at 8:37 PM Jeevan Ladhe > <jeevan.la...@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. No issues. I am ok either way. Regards, Jeevan Ladhe
