Re: Include WAL in base backup

On Wed, Jan 26, 2011 at 5:17 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> We should, and the easiest way is to actually use XLogRead() since the
> code is already there. How about the way in this patch?

Thanks for the update. I reread the patch.

+ MemSet(&statbuf, 0, sizeof(statbuf));
+ statbuf.st_mode=S_IRUSR | S_IWUSR;
+#ifndef WIN32
+ statbuf.st_uid=getuid();
+ statbuf.st_gid=getgid();
+#endif
+ statbuf.st_size=XLogSegSize;
+ statbuf.st_mtime=time(NULL);

Can you put a space around "="?

In the multiple-backups patch, Heikki uses geteuid and getegid for
the same purpose instead of getuid and getgid, as follows.

! /* Windows doesn't have the concept of uid and gid */
! #ifdef WIN32
! statbuf.st_uid = 0;
! statbuf.st_gid = 0;
! #else
! statbuf.st_uid = geteuid();
! statbuf.st_gid = getegid();
! #endif
! statbuf.st_mtime = time(NULL);
! statbuf.st_mode = S_IRUSR | S_IWUSR;
! statbuf.st_size = len;

Which is correct? Since we cannot start the PostgreSQL when
getuid != geteuid, I don't think that there is really difference between
getuid and geteuid. But other code always uses geteuid, so I think
that geteuid should be used here instead of getuid for the sake of
consistency.

OTOH, I'm not sure if there is really difference between getgid and
getegid in the backend (though I guess getgid == getegid because
getuid == geteuid), and which should be used here.
What is your opinion?

+ XLogFileName(xlogname, ThisTimeLineID, logid, logseg);
+
+ sprintf(fn, "./pg_xlog/%s", xlogname);
+ _tarWriteHeader(fn, NULL, &statbuf);

Can we use XLogFilePath? instead? Because sendFile has not been
used.

+ XLByteToSeg(endptr, endlogid, endlogseg);
<snip>
+ /* Have we reached our stop position yet? */
+ if (logid > endlogid ||
+ (logid == endlogid && logseg >= endlogseg))
+ break;

What I said in upthread is wrong. We should use XLByteToPrevSeg
for endptr and check "logseg > endlogseg". Otherwise, if endptr is
not a boundary byte, endlogid/endlogseg indicates the last
necessary WAL file, but it's not sent.

+ if (percent > 100)
+ percent = 100;

I'm not sure if this is good idea because the total received can go
further than the estimated size though the percentage stops at 100.
This looks more confusing than the previous behavior. Anyway,
I think that we need to document about the combination of -P and
-x in pg_basebackup.

In pg_basebackup.sgml
<term><option>--checkpoint <replaceable
class="parameter">fast|spread</replaceable></option></term>

Though this is not the problem of the patch, I found the inconsistency
of the descriptions about options of pg_basebackup. We should
s/--checkpoint/--checkpoint=

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center