Re: Include WAL in base backup

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