Re: Include WAL in base backup

On Thu, Jan 27, 2011 at 05:44, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> 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 "="?

I'll run pgindent, it'll take care of that.

> 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.

Yeah, changed for consistency.

> +                       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.

We can now, changed.

> 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.

Yeah, thanks for this - and thanks to Heikki for straightening it out for me.

> +       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.

I found it less confusing - but still somewhat confusing. I'll add some docs.

> 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=

Agreed, changed.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/