On Fri, Dec 30, 2016 at 6:41 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
> On Thu, Dec 29, 2016 at 6:13 PM, Magnus Hagander <mag...@hagander.net>
> wrote:
> > On Thu, Dec 29, 2016 at 12:35 AM, Michael Paquier
> > <michael.paqu...@gmail.com> wrote:
> >> On Wed, Dec 28, 2016 at 9:31 PM, Magnus Hagander <mag...@hagander.net>
> >> wrote:
> >> >> - I have switched the directory method to use a file pointer instead
> >> >> of a file descriptor as gzwrite returns int as the number of
> >> >> uncompressed bytes written.
> >> >
> >> > I don't really follow that reasoning :) Why does the directory method
> >> > have
> >> > to change to use a filepointer because of that?
> >>
> >> The only reason is that write() returns size_t and fwrite returns int,
> >> while gzwrite() returns int. It seems more consistent to use fwrite()
> >> in this case. Or we don't bother about my nitpicking and just cast
> >> stuff.
> >
> >
> > I can at least partially see that argument, but your patch doesn't
> actually
> > use fwrite(), it uses write() with fileno()...
>
> That was part of the one/two things I wanted to change before sending
> a fresh patch.
>
OK.
> > But also, on my platform (debian jessie), fwrite() returns size_t, and
> > write() returns ssize_t. So those are apparently both different from what
> > your platform does - which one did you get that one?
>
> It looks like I misread the macos man pages previously. Thay actually
> list ssize_t. I find a bit surprising the way gzwrite is designed. It
> uses an input an unsigned integer and returns to caller a signed
> integer, so this will never work with uncompressed buffers of sizes
> higher than 2GB. There's little point to worry about that in
> pg_receivexlog though, so let's just cast to ssize_t.
>
> Attached is a simplified new version, I have kept the file descriptor
> as originally done. Note that tests are actually difficult to work
> out, there is no way to run in batch pg_receivexlog..
>
A few further notes:
You are using the filemode to gzopen and the mode_compression variable to
set the compression level. The pre-existing code in pg_basebackup uses
gzsetparams(). Is there a particular reason you didn't do it the same way?
Small comment:
- if (pad_to_size)
+ if (pad_to_size && dir_data->compression == 0)
{
/* Always pre-pad on regular files */
That "always" is not true anymore. Commit-time cleanup can be done of that.
The rest of this looks good to me, but please comment on the gzopen part
before we proceed to commit :)
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
