Re: Your review of pg_receivexlog/pg_basebackup

On Tue, Nov 1, 2011 at 3:08 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Fri, Oct 28, 2011 at 08:46, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Thu, Oct 27, 2011 at 11:14 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>> Here's a version that does this. Turns out this requires a lot less
>>> code than what was previously in there, which is always nice.
>>>
>>> We still need to solve the other part which is how to deal with the
>>> partial files on restore. But this is definitely a cleaner way from a
>>> pure pg_receivexlog perspective.
>>>
>>> Comments/reviews?
>>
>> Looks good.
>>
>> Minor comment:
>> the source code comment of FindStreamingStart() seems to need to be updated.
>
> Here's an updated patch that both includes this update to the comment,
> and also the functionality to pre-pad files to 16Mb. This also seems
> to have simplified the code, which is a nice bonus.

Here are the comments:

In open_walfile(), "zerobuf" needs to be free'd after use of it.

+ f = open(fn, O_WRONLY | O_CREAT | PG_BINARY, 0666);

We should use "S_IRUSR | S_IWUSR" instead of "0666" as a file access modes?

+ if (write(f, zerobuf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
+ {
+ fprintf(stderr, _("%s: could not pad WAL segment %s: %s\n"),
+ progname, fn, strerror(errno));
+ close(f);
+ return -1;
+ }

When write() fails, we should delete the partial WAL file, like
XLogFileInit() does?
If not, subsequent pg_receivexlog always fails unless a user deletes
it manually.
Because open_walfile() always fails when it finds an existing partial WAL file.

When open_walfile() fails, pg_receivexlog exits without closing the connection.
I don't think this is good error handling. But this issue itself is
not what we're
trying to address now. So I think you can commit separately from current patch.

Regards,

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