From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Stephen Frost <sfrost(at)snowman(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Include WAL in base backup |
Date: | 2011-01-30 19:22:02 |
Message-ID: | AANLkTinTVnX9OekxOBGK5=0j2BAEBQu5Ju=XEbUn+_XJ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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/
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2011-01-30 19:28:41 | updated patch for foreach stmt |
Previous Message | Robert Haas | 2011-01-30 19:12:12 | Re: multiset patch review |