Re: pg_basebackup stream xlog to tar

On Thu, Sep 1, 2016 at 9:19 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Thu, Sep 1, 2016 at 6:58 AM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
> > Attached patch adds support for -X stream to work with .tar and .tar.gz
> file
> > formats.
>
> Nice.
>
> > If tar mode is specified, a separate pg_xlog.tar (or .tar.gz) file is
> > created and the data is streamed into it. Regular mode is (should not)
> see
> > any changes in how it works.
>
> Could you use XLOGDIR from xlog_internal.h at least?
>

Yes, that makes sense.

> > The implementation creates a "walmethod" for directory and one for tar,
> > which is basically a set of function pointers that we pass around as
> part of
> > the StreamCtl structure. All calls to modify the files are sent through
> the
> > current method, using the normal open/read/write calls as it is now for
> > directories, and the more complicated method for tar and targz.
>
> That looks pretty cool by looking at the code.
>
> > The tar method doesn't support all things that are required for
> > pg_receivexlog, but I don't think it makes any real sense to have support
> > for pg_receivexlog in tar mode. But it does support all the things that
> > pg_basebackup needs.
>
> Agreed. Your patch is enough complicated.
>
> > Some smaller pieces of functionality like unlinking files on failure and
> > padding files have been moved into the walmethod because they have to be
> > differently implemented (we cannot pre-pad a compressed file -- the size
> > will depend on the compression ration anyway -- for example).
> >
> > AFAICT we never actually documented that -X stream doesn't work with tar
> in
> > the manpage of current versions. Only in the error message. We might
> want to
> > fix that in backbranches.
>
> +1 for documenting that in back-branches.
>
> > In passing this also fixes an XXX comment about not re-lseeking on the
> WAL
> > file all the time -- the walmethod now tracks the current position in the
> > file in a variable.
> >
> > Finally, to make this work, the pring_tar_number() function is now
> exported
> > from port/tar.c along with the other ones already exported from there.
>
> walmethods.c:387:9: warning: comparison of unsigned expression < 0 is
> always false [-Wtautological-compare]
> if (r < 0)
> This patch is generating a warning for me with clang.
>
> I have just tested this feature:
> $ pg_basebackup -X stream -D data -F t
> Which generates that:
> $ ls data
> base.tar pg_xlog.tar
> However after decompressing pg_xlog.tar the segments don't have a correct
> size:
> $ ls -l 0*
> -rw------- 1 mpaquier _guest 3.9M Sep 1 16:12 000000010000000000000011
>
> Even if that's filling them with zeros during pg_basebackup when a
> segment is done, those should be 16MB to allow users to reuse them
> directly.
>

Huh, that's annoying. I must've broken that when I fixed padding for
compressed files. It forgets the padding when it updates the size of the
tarfile (works fine for compressed files because padding is done at the
end).

That's definitely not intended - it's supposed to be 16Mb. And it actually
writes 16Mb to the tarfile, it's the extraction that doesn't see them. That
also means that if you get more than one member of the tarfile at this
point, it will be corrupt. (It's not corrupt in the .tar.gz case, clearly
my testing of the very last iteration of the patch forgot to doublecheck
this with both).

Oops. Will fix.

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