Re: pg_basebackup stream xlog to tar

On Tue, Oct 4, 2016 at 12:05 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> (Squashing two emails into one)
>
> On Fri, Sep 30, 2016 at 11:16 PM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
> > And here's yet another version, now rebased on top of the fsync and
> nosync
> > changes that got applied.
>
> My fault :p
>

Yes, definitely :P

> > In particular, this conflicted with pretty much every single change from
> the
> > fsync patch, so I'm definitely looking for another round of review before
> > this can be committed.
>
> Could you rebase once again? This is conflicting with the recent
> changes in open_walfile() and close_walfile() of 728ceba.
>

Done.

> > I ended up moving much of the fsync stuff into walmethods.c, since they
> were
> > dependent on if we used tar or not (obviously only the parts about the
> wal,
> > not the basebackup). So there's a significant risk that I missed
> something
> > there.
>
> + /* If we have a temp prefix, normal is we rename the file */
> + snprintf(tmppath, sizeof(tmppath), "%s/%s%s",
> + dir_data->basedir, df->pathname, df->temp_suffix);
> This comment could be improved, like "normal operation is to rename
> the file" for example.
>

Agreed and fixed.

> + if (lseek(fd, 0, SEEK_SET) != 0)
> + return NULL;
> [...]
> + if (fsync_fname(f->fullpath, false, progname) != 0)
> + return NULL;
> + if (fsync_parent_path(f->fullpath, progname) != 0)
> + return NULL;
> fd leaks for those three code paths. And when one of those fsync calls
> fail the previously pg_malloc'd f leaks as well. It may be a good idea
> to have a single routine doing all the pg_free work for
> DirectoryMethodFile. You'll need it as well in dir_close(). Or even
> better: do the fsync calls before allocating f. For pg_basebackup it
> does not matter much, but it does for pg_receivexlog that has a retry
> logic.
>

Agreed, moving the fsyncs is definitely the best there.

> + if (deflateInit2(tar_data->zp, tar_data->compression,
> Z_DEFLATED, 15 + 16, 8, Z_DEFAULT_STRATEGY) != Z_OK)
> + {
> + tar_set_error("deflateInit2 failed");
> + return NULL;
> + }
> tar_data->zp leaks here.. Perhaps that does not matter as tar mode is
> just used by pg_basebackup now but if we introduce some retry logic
> I'd prefer avoiding any problems in the future.
>

Agreed, leaks are bad even if they are not a direct problem right now.
Fixed.

> On Thu, Sep 29, 2016 at 7:44 PM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
> >>
> >> static bool
> >> existsTimeLineHistoryFile(StreamCtl *stream)
> >> {
> >> [...]
> >> + return stream->walmethod->existsfile(histfname);
> >> }
> >> existsfile returns always false for the tar method. This does not
> >> matter much because pg_basebackup exists immediately in case of a
> >> failure, but I think that this deserves a comment in ReceiveXlogStream
> >> where existsTimeLineHistoryFile is called.
> >
> > OK, added. As you say, the behaviour is expected, but it makes sense to
> > mention it clealry there.
>
> Thanks.
> + * false, as we are never streaming into an existing file and
> therefor
> s/therefor/therefore.
>

Fixed.

> > So what's our basic rule for these perl tests - are we allowed to use
> > pg_xlogdump from within a pg_basebackup test? If so that could actually
> be a
> > useful test - do the backup, extract the xlog and verify that it contains
> > the SWITCH record.
>
> pg_xlogdump is part of the default temporary installation, so using it
> is fine. The issue though is how do we untar pg_xlog.tar without a
> native perl call? That's not present down to 5.8.8.. The test you are
> proposing in 010_pg_basebackup.pl is the best we can do for now.
>
>
My initial thought was actually adding that check to non-tar format.

But I agree, to test the tar format things specifically we *somehow* need
to be able to untar. We either need to rely on a system tar (which will
likely break badly on Windows) or we need to rely on a perl tar module.

But independent of this patch, actually putting that test in for non-tar
mode would probably not be a bad idea -- if that breaks, it's likely both
break, after all.

Thanks!

//Magnus