Re: pg_basebackup stream xlog to tar - Mailing list pgsql-hackers

(Squashing two emails into one)

On Fri, Sep 30, 2016 at 11:16 PM, Magnus Hagander <magnus@hagander.net> wrote:
> And here's yet another version, now rebased on top of the fsync and nosync
> changes that got applied.

My fault :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.

> 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.

+       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.

+           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.

On Thu, Sep 29, 2016 at 7:44 PM, Magnus Hagander <magnus@hagander.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.

> 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.