Re: Include WAL in base backup

On Tue, Jan 25, 2011 at 12:33 AM, Magnus Hagander <magnus(at)hagander(dot)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