On Tue, Jan 25, 2011 at 12:33 AM, Magnus Hagander <magnus@hagander.net> wrote:
> Here's an updated version of the patch:
>
> * rebased on the current git master (including changing the switch
> from -w to -x since -w was used now)
> * includes some documentation
> * use XLogByteToSeg and uint32 as mentioned here
> * refactored to remove SendBackupDirectory(), removing the ugly closetar boolean
I reviewed the patch. Here are comments.
+ {"xlog", no_argument, NULL, 'w'},
Typo: s/w/x
/** BASE_BACKUP [LABEL <label>] [PROGRESS] [FAST]*/
In repl_gram.y, the above comment needs to be updated.
Both this patch and the multiple-backup one removes SendBackupDirectory
in the almost same way. So, how about committing that common part first?
+ XLByteToSeg(endptr, endlogid, endlogseg);
XLByteToPrevSeg should be used for the backup end location. Because
when it's a boundary byte, all the required WAL records exist in the
previous WAL segment. This is why pg_stop_backup uses XLByteToPrevSeg
for the backup end location.
+ elog(DEBUG1, "Going to send wal from %i.%i to %i.%i",
+ logid, logseg,
+ endlogid, endlogseg);
%u should be used instead of %i. Or how about logging the filename?
+ char xlogname[64];
How about using MAXFNAMELEN instead of 64?
+ XLogFileName(xlogname, ThisTimeLineID, logid, logseg);
+ sprintf(fn, "./pg_xlog/%s", xlogname);
How about using XLogFilePath? instead?
+ if (logid > endptr.xlogid ||
+ (logid == endptr.xlogid && logseg >= endptr.xrecoff / XLogSegSize))
Why don't you use endlogseg?
The estimated size of $PGDATA doesn't include WAL files, but the
actual one does.
This inconsistency messes up the progress report as follows:
33708/17323 kB (194%) 1/1 tablespaces
So, what about sparating the progress report into two parts: one for $PGDATA and
tablespaces, another for WAL files?
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
