Обсуждение: pg_basebackup stream xlog to tar
Attached patch adds support for -X stream to work with .tar and .tar.gz file formats.
--
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.
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.
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.
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.
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.
Вложения
On Thu, Sep 1, 2016 at 6:58 AM, Magnus Hagander <magnus@hagander.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? > 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. -- Michael
On Thu, Sep 1, 2016 at 9:19 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
Yes, that makes sense.
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).
-- On Thu, Sep 1, 2016 at 6:58 AM, Magnus Hagander <magnus@hagander.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.
On Thu, Sep 1, 2016 at 4:41 PM, Magnus Hagander <magnus@hagander.net> wrote: > 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. If at the same time you could add some tests in pg_basebackup/t, that would be great :) -- Michael
On Thu, Sep 1, 2016 at 9:43 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Thu, Sep 1, 2016 at 4:41 PM, Magnus Hagander <magnus@hagander.net> wrote:
> 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.
If at the same time you could add some tests in pg_basebackup/t, that
would be great :)
That's definitely on my slightly-longer-term plan. But I've successfully managed to avoid perl long enough now that looking at the code in those tests is mighty confusing :) So I need a bunch of readup before I can figure that out. (yes, that means I've managed to avoid our own discussions about that style tests on this list quite successfully too :P)
We don't seem to check for similar issues as the one just found in the existing tests though, do we? As in, we don't actually verify that the xlog files being streamed are 16Mb? (Or for that matter that the tarfile emitted by -Ft is actually a tarfile?) Or am I missing some magic somewhere? :)
On Thu, Sep 1, 2016 at 5:13 PM, Magnus Hagander <magnus@hagander.net> wrote: > We don't seem to check for similar issues as the one just found in the > existing tests though, do we? As in, we don't actually verify that the xlog > files being streamed are 16Mb? (Or for that matter that the tarfile emitted > by -Ft is actually a tarfile?) Or am I missing some magic somewhere? :) No. There is no checks on the WAL file size (you should use the output of pg_controldata to see how large the segments should be). For the tar file, the complication is in its untar... Perl provides some ways to untar things, though the oldest version that we support in the TAP tests does not offer that :( -- Michael
On Thu, Sep 1, 2016 at 2:39 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Thu, Sep 1, 2016 at 5:13 PM, Magnus Hagander <magnus@hagander.net> wrote:
> We don't seem to check for similar issues as the one just found in the
> existing tests though, do we? As in, we don't actually verify that the xlog
> files being streamed are 16Mb? (Or for that matter that the tarfile emitted
> by -Ft is actually a tarfile?) Or am I missing some magic somewhere? :)
No. There is no checks on the WAL file size (you should use the output
of pg_controldata to see how large the segments should be). For the
tar file, the complication is in its untar... Perl provides some ways
to untar things, though the oldest version that we support in the TAP
tests does not offer that :(
Ugh. That would be nice to have, but I think that's outside the scope of this patch.
PFA is an updated version of this patch that:
* documents a magic value passed to zlib (which is in their documentation as being a magic value, but has no define)
* documents a magic value passed to zlib (which is in their documentation as being a magic value, but has no define)
* fixes the padding of tar files
* adds a most basic test that the -X stream -Ft does produce a tarfile
As for using XLOGDIR to drive the name of the tarfile. pg_basebackup is already hardcoded to use pg_xlog. And so are the tests. We probably want to fix that, but that's a separate step and this patch will be easier to review and test if we keep it out for now.
Вложения
On Sat, Sep 3, 2016 at 10:35 PM, Magnus Hagander <magnus@hagander.net> wrote:
> Ugh. That would be nice to have, but I think that's outside the scope of
> this patch.
A test for this patch that could have value would be to use
pg_basebackup -X stream -Ft, then untar pg_xlog.tar and look at the
size of the segments. If you have an idea to untar something without
the in-core perl support because we need to have the TAP stuff able to
run on at least 5.8.8, I'd welcome an idea. Honestly I still have
none, and that's why the recovery tests do not use tarballs in their
tests when using pg_basebackup. In short let's not add something more
for this patch.
> PFA is an updated version of this patch that:
> * documents a magic value passed to zlib (which is in their documentation as
> being a magic value, but has no define)
> * fixes the padding of tar files
> * adds a most basic test that the -X stream -Ft does produce a tarfile
So I had a more serious look at this patch, and it basically makes
more generic the operations done for the plain mode by adding a set of
routines that can be used by both tar and plain mode to work on the
WAL files streamed. Elegant.
+ <para>
+ The transaction log files will be written to
+ the <filename>base.tar</filename> file.
+ </para>
Nit: number of spaces here.
-mark_file_as_archived(const char *basedir, const char *fname)
+mark_file_as_archived(StreamCtl *stream, const char *fname)
Just passing WalMethod as argument would be enough, but... My patch
adding the fsync calls to pg_basebackup could just make use of
StreamCtl, so let's keep it as you suggest.
static boolexistsTimeLineHistoryFile(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.
I find the use of existsfile() in open_walfile() rather confusing
because this relies on the fact that existsfile() returns always
false for the tar mode. We could add an additional field in WalMethod
to store the method type and use that more, but that may make the code
more confusing than what you propose. What do you think?
+ int (*unlink) (const char *pathname);
The unlink method is used nowhere. This could just be removed.
-static void
+voidprint_tar_number(char *s, int len, uint64 val)
This could be an independent patch. Or not.
I think that I found another bug regarding the contents of the
segments. I did pg_basebackup -F t -X stream, then untared pg_xlog.tar
which contained segment 1/0/2, then:
$ pg_xlogdump 000000010000000000000002
pg_xlogdump: FATAL: could not find a valid record after 0/2000000
I'd expect this segment to have records, up to a XLOG_SWITCH record.
> As for using XLOGDIR to drive the name of the tarfile. pg_basebackup is
> already hardcoded to use pg_xlog. And so are the tests. We probably want to
> fix that, but that's a separate step and this patch will be easier to review
> and test if we keep it out for now.
Yes. pg_basebackup is not the only thing here missing the point here,
here is most of the list:
$ git grep "pg_xlog\"" -- *.c *.h
src/backend/access/transam/xlog.c:static const char *xlogSourceNames[]
= {"any", "archive", "pg_xlog", "stream"};
src/backend/replication/basebackup.c: dir = AllocateDir("pg_xlog");
src/backend/replication/basebackup.c: (errmsg("could
not open directory \"%s\": %m", "pg_xlog")));
src/backend/replication/basebackup.c: while ((de = ReadDir(dir,
"pg_xlog")) != NULL)
src/backend/replication/basebackup.c: if (strcmp(pathbuf,
"./pg_xlog") == 0)
src/backend/storage/file/fd.c: if (lstat("pg_xlog", &st) < 0)
src/backend/storage/file/fd.c: "pg_xlog")));
src/backend/storage/file/fd.c: if (pgwin32_is_junction("pg_xlog"))
src/backend/storage/file/fd.c: walkdir("pg_xlog", pre_sync_fname,
false, DEBUG1);
src/backend/storage/file/fd.c: walkdir("pg_xlog",
datadir_fsync_fname, false, LOG);
src/bin/initdb/initdb.c: snprintf(pg_xlog, MAXPGPATH, "%s/pg_xlog", pg_data);
src/bin/initdb/initdb.c: subdirloc = psprintf("%s/pg_xlog", pg_data);
src/bin/pg_basebackup/pg_basebackup.c: snprintf(param->xlogdir,
sizeof(param->xlogdir), "%s/pg_xlog", basedir);
src/bin/pg_basebackup/pg_basebackup.c: if
(!((pg_str_endswith(filename, "/pg_xlog") ||
src/bin/pg_basebackup/pg_basebackup.c: linkloc =
psprintf("%s/pg_xlog", basedir);
src/bin/pg_rewind/copy_fetch.c: strcmp(path, "pg_xlog") == 0)
src/bin/pg_rewind/filemap.c: if (strcmp(path, "pg_xlog") == 0 &&
type == FILE_TYPE_SYMLINK)
src/bin/pg_rewind/filemap.c: if (exists &&
!S_ISDIR(statbuf.st_mode) && strcmp(path, "pg_xlog") != 0)
src/bin/pg_rewind/filemap.c: if (strcmp(path, "pg_xlog") == 0 &&
type == FILE_TYPE_SYMLINK)
src/bin/pg_upgrade/exec.c: "pg_xlog"};
src/include/access/xlog_internal.h:#define XLOGDIR "pg_xlog"
Thanks,
--
Michael
On Mon, Sep 5, 2016 at 4:01 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > [ review comments ] This thread has been sitting idle for more than 3 weeks, so I'm marking it "Returned with Feedback" in the CommitFest application. Magnus, Michael's latest round of comments seem pretty trivial, so perhaps you want to just fix whichever of them seem to you to have merit and commit without waiting for the next CommitFest. Or, you can resubmit for the next CommitFest if you think it needs more review. But the CommitFest is just about over so it's time to clean out old entries, one way or the other. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
<p dir="ltr"><p dir="ltr">On Sep 28, 2016 19:11, "Robert Haas" <<a href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>>wrote:<br /> ><br /> > On Mon, Sep 5, 2016 at 4:01AM, Michael Paquier<br /> > <<a href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>> wrote:<br/> > > [ review comments ]<br /> ><br /> > This thread has been sitting idle for more than 3 weeks,so I'm<br /> > marking it "Returned with Feedback" in the CommitFest application.<br /> > Magnus, Michael's latestround of comments seem pretty trivial, so<br /> > perhaps you want to just fix whichever of them seem to you tohave<br /> > merit and commit without waiting for the next CommitFest. Or, you can<br /> > resubmit for the nextCommitFest if you think it needs more review.<br /> > But the CommitFest is just about over so it's time to cleanout old<br /> > entries, one way or the other.<br /><p dir="ltr">Yeah, understood. I was planning to get back toit this week, but failed to find the time. I'll still have some hope about later this week, but most likely not until thenext. <p dir="ltr">/Magnus
On Mon, Sep 5, 2016 at 10:01 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Sat, Sep 3, 2016 at 10:35 PM, Magnus Hagander <magnus@hagander.net> wrote:
> Ugh. That would be nice to have, but I think that's outside the scope of
> this patch.
A test for this patch that could have value would be to use
pg_basebackup -X stream -Ft, then untar pg_xlog.tar and look at the
size of the segments. If you have an idea to untar something without
the in-core perl support because we need to have the TAP stuff able to
run on at least 5.8.8, I'd welcome an idea. Honestly I still have
none, and that's why the recovery tests do not use tarballs in their
tests when using pg_basebackup. In short let's not add something more
for this patch.
> PFA is an updated version of this patch that:
> * documents a magic value passed to zlib (which is in their documentation as
> being a magic value, but has no define)
> * fixes the padding of tar files
> * adds a most basic test that the -X stream -Ft does produce a tarfile
So I had a more serious look at this patch, and it basically makes
more generic the operations done for the plain mode by adding a set of
routines that can be used by both tar and plain mode to work on the
WAL files streamed. Elegant.
+ <para>
+ The transaction log files will be written to
+ the <filename>base.tar</filename> file.
+ </para>
Nit: number of spaces here.
Fixed.
-mark_file_as_archived(const char *basedir, const char *fname)
+mark_file_as_archived(StreamCtl *stream, const char *fname)
Just passing WalMethod as argument would be enough, but... My patch
adding the fsync calls to pg_basebackup could just make use of
StreamCtl, so let's keep it as you suggest.
Yeah, I think it's cleaner to pass the whole structure around really. If not now, we'd need it eventually. That makes all callers more consistent.
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.
I find the use of existsfile() in open_walfile() rather confusing
because this relies on the fact that existsfile() returns always
false for the tar mode. We could add an additional field in WalMethod
to store the method type and use that more, but that may make the code
more confusing than what you propose. What do you think?
Yeah, I'm not sure that helps. The point is that the abstraction is supposed to take care of that. But if it's confusing, then clearly a comment is warranted there, so I've added that. Do you think that makes it clear enough?
+ int (*unlink) (const char *pathname);
The unlink method is used nowhere. This could just be removed.
That's clearly a missed cleanup. Removed, thanks.
-static void
+void
print_tar_number(char *s, int len, uint64 val)
This could be an independent patch. Or not.
Could be, but we don't really have any other uses for it.
I think that I found another bug regarding the contents of the
segments. I did pg_basebackup -F t -X stream, then untared pg_xlog.tar
which contained segment 1/0/2, then:
$ pg_xlogdump 000000010000000000000002
pg_xlogdump: FATAL: could not find a valid record after 0/2000000
I'd expect this segment to have records, up to a XLOG_SWITCH record.
Ugh. That's definitely broken yes. It seeked back and overwrote the tar header with the data, instead of starting where the file part was supposed to be. It worked fine on compressed files, and it's when implementing that that it broke.
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.
I also noticed that using -Z5 created a .tar.gz and -z created a .tar (which was compressed). Because compresslevel is set to -1 with -z, meaning default.
Again, apologies for getting late back into the game here.
//Magnus
Вложения
On Thu, Sep 29, 2016 at 12:44 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.On Mon, Sep 5, 2016 at 10:01 AM, Michael Paquier <michael.paquier@gmail.com> wrote:On Sat, Sep 3, 2016 at 10:35 PM, Magnus Hagander <magnus@hagander.net> wrote:
> Ugh. That would be nice to have, but I think that's outside the scope of
> this patch.
A test for this patch that could have value would be to use
pg_basebackup -X stream -Ft, then untar pg_xlog.tar and look at the
size of the segments. If you have an idea to untar something without
the in-core perl support because we need to have the TAP stuff able to
run on at least 5.8.8, I'd welcome an idea. Honestly I still have
none, and that's why the recovery tests do not use tarballs in their
tests when using pg_basebackup. In short let's not add something more
for this patch.
> PFA is an updated version of this patch that:
> * documents a magic value passed to zlib (which is in their documentation as
> being a magic value, but has no define)
> * fixes the padding of tar files
> * adds a most basic test that the -X stream -Ft does produce a tarfile
So I had a more serious look at this patch, and it basically makes
more generic the operations done for the plain mode by adding a set of
routines that can be used by both tar and plain mode to work on the
WAL files streamed. Elegant.
+ <para>
+ The transaction log files will be written to
+ the <filename>base.tar</filename> file.
+ </para>
Nit: number of spaces here.Fixed.-mark_file_as_archived(const char *basedir, const char *fname)
+mark_file_as_archived(StreamCtl *stream, const char *fname)
Just passing WalMethod as argument would be enough, but... My patch
adding the fsync calls to pg_basebackup could just make use of
StreamCtl, so let's keep it as you suggest.Yeah, I think it's cleaner to pass the whole structure around really. If not now, we'd need it eventually. That makes all callers more consistent.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.I find the use of existsfile() in open_walfile() rather confusing
because this relies on the fact that existsfile() returns always
false for the tar mode. We could add an additional field in WalMethod
to store the method type and use that more, but that may make the code
more confusing than what you propose. What do you think?Yeah, I'm not sure that helps. The point is that the abstraction is supposed to take care of that. But if it's confusing, then clearly a comment is warranted there, so I've added that. Do you think that makes it clear enough?
+ int (*unlink) (const char *pathname);
The unlink method is used nowhere. This could just be removed.That's clearly a missed cleanup. Removed, thanks.
-static void
+void
print_tar_number(char *s, int len, uint64 val)
This could be an independent patch. Or not.Could be, but we don't really have any other uses for it.I think that I found another bug regarding the contents of the
segments. I did pg_basebackup -F t -X stream, then untared pg_xlog.tar
which contained segment 1/0/2, then:
$ pg_xlogdump 000000010000000000000002
pg_xlogdump: FATAL: could not find a valid record after 0/2000000
I'd expect this segment to have records, up to a XLOG_SWITCH record.Ugh. That's definitely broken yes. It seeked back and overwrote the tar header with the data, instead of starting where the file part was supposed to be. It worked fine on compressed files, and it's when implementing that that it broke.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.I also noticed that using -Z5 created a .tar.gz and -z created a .tar (which was compressed). Because compresslevel is set to -1 with -z, meaning default.Again, apologies for getting late back into the game here.
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.
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.
--
Вложения
(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.
--
Michael
On Tue, Oct 4, 2016 at 12:05 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
(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
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@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.
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
Вложения
On Sat, Oct 15, 2016 at 8:51 AM, Magnus Hagander <magnus@hagander.net> wrote:
> Fixed.
Ok, I had a extra look on the patch:
+ <para>The transactionn log files are written to a separate file
+ called <filename>pg_xlog.tar</filename>.
+ </para>
s/transactionn/transaction/, and the <para> markup should be on its own line.
+ if (dir_data->sync)
+ {
+ if (fsync_fname(tmppath, false, progname) != 0)
+ {
+ close(fd);
+ return NULL;
+ }
+ if (fsync_parent_path(tmppath, progname) != 0)
+ {
+ close(fd);
+ return NULL;
+ }
+ }
Nit: squashing both things together would simplify the code.
+ else if (method == CLOSE_UNLINK
+ )
Your finger slipped here.
Except that it looks in pretty good to me, so I am switching that as
ready for committer.
> 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.
Agreed (you were able to break only tar upthread with your patch). One
way to do that elegantly would be to:
1) extend slurp_dir to return only files that have a matching pattern.
That's not difficult to do:
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -184,10 +184,14 @@ sub generate_ascii_string
sub slurp_dir{
- my ($dir) = @_;
+ my ($dir, $match_pattern) = @_; opendir(my $dh, $dir) or die "could not opendir \"$dir\": $!"; my
@direntries= readdir $dh;
+ if (defined($match_pattern))
+ {
+ @direntries = grep($match_pattern, @direntries);
+ } closedir $dh; return @direntries;}
Sorting them at the same time may be a good idea..
2) Add an option to pg_xlogdump to be able to output its output to a
file. That would be awkward to rely on grabbing the output data from a
pipe... On Windows particularly. Thinking about it, would that
actually be useful to others? That's not a complicated patch.
--
Michael
On Mon, Oct 17, 2016 at 2:37 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > Except that it looks in pretty good to me, so I am switching that as > ready for committer. + /* + * Create pg_xlog/archive_status (and thus pg_xlog) so we can write to + * basedir/pg_xlog as the directory entry in the tar file may arrive + * later. + */ + snprintf(statusdir, sizeof(statusdir), "%s/pg_xlog/archive_status", + basedir); This part conflicts with f82ec32, where you need make pg_basebackup aware of the backend version.. I promise that's the last conflict, at least I don't have more patches planned in the area. -- Michael
On Fri, Oct 21, 2016 at 2:02 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
Thanks for your repeated reviews!
On Mon, Oct 17, 2016 at 2:37 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Except that it looks in pretty good to me, so I am switching that as
> ready for committer.
+ /*
+ * Create pg_xlog/archive_status (and thus pg_xlog) so we can write to
+ * basedir/pg_xlog as the directory entry in the tar file may arrive
+ * later.
+ */
+ snprintf(statusdir, sizeof(statusdir), "%s/pg_xlog/archive_status",
+ basedir);
This part conflicts with f82ec32, where you need make pg_basebackup
aware of the backend version.. I promise that's the last conflict, at
least I don't have more patches planned in the area.
It also broke the tests and invalidated some documentation. But it was easy enough to fix.
I've now applied this, so next time you get to do the merging :P Joking aside, please review and let me know if you can spot something I messed up in the final merge.
Thanks for your repeated reviews!
On Mon, Oct 17, 2016 at 7:37 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Sat, Oct 15, 2016 at 8:51 AM, Magnus Hagander <magnus@hagander.net> wrote:
> Fixed.
Ok, I had a extra look on the patch:
+ <para>The transactionn log files are written to a separate file
+ called <filename>pg_xlog.tar</filename>.
+ </para>
s/transactionn/transaction/, and the <para> markup should be on its own line.
+ if (dir_data->sync)
+ {
+ if (fsync_fname(tmppath, false, progname) != 0)
+ {
+ close(fd);
+ return NULL;
+ }
+ if (fsync_parent_path(tmppath, progname) != 0)
+ {
+ close(fd);
+ return NULL;
+ }
+ }
Nit: squashing both things together would simplify the code.
+ else if (method == CLOSE_UNLINK
+ )
Your finger slipped here.
Except that it looks in pretty good to me, so I am switching that as
ready for committer.
I incorporated those changes before pushing.
> 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.
Agreed (you were able to break only tar upthread with your patch). One
way to do that elegantly would be to:
1) extend slurp_dir to return only files that have a matching pattern.
That's not difficult to do:
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -184,10 +184,14 @@ sub generate_ascii_string
sub slurp_dir
{
- my ($dir) = @_;
+ my ($dir, $match_pattern) = @_;
opendir(my $dh, $dir)
or die "could not opendir \"$dir\": $!";
my @direntries = readdir $dh;
+ if (defined($match_pattern))
+ {
+ @direntries = grep($match_pattern, @direntries);
+ }
closedir $dh;
return @direntries;
}
Sorting them at the same time may be a good idea..
2) Add an option to pg_xlogdump to be able to output its output to a
file. That would be awkward to rely on grabbing the output data from a
pipe... On Windows particularly. Thinking about it, would that
actually be useful to others? That's not a complicated patch.
I think both of those would be worthwhile. Just for the testability in itself, but such a flag to pg_xlogdump would probably be useful in other cases as well, beyond just the testing.
On Sun, Oct 23, 2016 at 10:30 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Mon, Oct 17, 2016 at 7:37 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>> > 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.
>>
>> Agreed (you were able to break only tar upthread with your patch). One
>> way to do that elegantly would be to:
>> 1) extend slurp_dir to return only files that have a matching pattern.
>> That's not difficult to do:
>> --- a/src/test/perl/TestLib.pm
>> +++ b/src/test/perl/TestLib.pm
>> @@ -184,10 +184,14 @@ sub generate_ascii_string
>>
>> sub slurp_dir
>> {
>> - my ($dir) = @_;
>> + my ($dir, $match_pattern) = @_;
>> opendir(my $dh, $dir)
>> or die "could not opendir \"$dir\": $!";
>> my @direntries = readdir $dh;
>> + if (defined($match_pattern))
>> + {
>> + @direntries = grep($match_pattern, @direntries);
>> + }
>> closedir $dh;
>> return @direntries;
>> }
>> Sorting them at the same time may be a good idea..
>> 2) Add an option to pg_xlogdump to be able to output its output to a
>> file. That would be awkward to rely on grabbing the output data from a
>> pipe... On Windows particularly. Thinking about it, would that
>> actually be useful to others? That's not a complicated patch.
>
> I think both of those would be worthwhile. Just for the testability in
> itself, but such a flag to pg_xlogdump would probably be useful in other
> cases as well, beyond just the testing.
Looking quickly at the code, it does not seem that complicated... I
may just send patches tomorrow for all those things and be done with
it, all that on its new dedicated thread.
--
Michael
On Sun, Oct 23, 2016 at 10:52 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Sun, Oct 23, 2016 at 10:30 PM, Magnus Hagander <magnus@hagander.net> wrote: >> I think both of those would be worthwhile. Just for the testability in >> itself, but such a flag to pg_xlogdump would probably be useful in other >> cases as well, beyond just the testing. > > Looking quickly at the code, it does not seem that complicated... I > may just send patches tomorrow for all those things and be done with > it, all that on its new dedicated thread. Actually not that much after noticing that pg_xlogdump emulates some of the backend's StringInfo routines and calls at the end vprintf() to output everything to stdout, which is ugly. The cleanest solution here would be to make StringInfo a bit more portable and allow them for frontends, somehting that may be useful for any utility playing with rm_desc. A less cleaner solution would be to somewhat store a fd pointing to a file (or stdout) into compat.c and output to it. I'd slightly prefer the first solution, but that does not seem worth the effort just for pg_xlogdump and one test. -- Michael
On 2016-10-17 14:37:05 +0900, Michael Paquier wrote: > 2) Add an option to pg_xlogdump to be able to output its output to a > file. That would be awkward to rely on grabbing the output data from a > pipe... On Windows particularly. Thinking about it, would that > actually be useful to others? That's not a complicated patch. Hm? Just redirecting output seems less complicated? And afaik works on windows as well? Greetings, Andres Freund
On Mon, Oct 24, 2016 at 1:38 PM, Andres Freund <andres@anarazel.de> wrote: > On 2016-10-17 14:37:05 +0900, Michael Paquier wrote: >> 2) Add an option to pg_xlogdump to be able to output its output to a >> file. That would be awkward to rely on grabbing the output data from a >> pipe... On Windows particularly. Thinking about it, would that >> actually be useful to others? That's not a complicated patch. > > Hm? Just redirecting output seems less complicated? And afaik works on > windows as well? In the TAP suite STDOUT is already redirect to the log files. Perhaps we could just do a SELECT FILE; to redirect the output of pg_xlogdump temporarily into a custom location just for the sake of a test like that. -- Michael
On Sun, Oct 23, 2016 at 10:28 PM, Magnus Hagander <magnus@hagander.net> wrote:
> It also broke the tests and invalidated some documentation. But it was easy
> enough to fix.
>
> I've now applied this, so next time you get to do the merging :P Joking
> aside, please review and let me know if you can spot something I messed up
> in the final merge.
Just had another look at it..
+static int
+tar_fsync(Walfile f)
+{
+ Assert(f != NULL);
+ tar_clear_error();
+
+ /*
+ * Always sync the whole tarfile, because that's all we can do. This makes
+ * no sense on compressed files, so just ignore those.
+ */
+ if (tar_data->compression)
+ return 0;
+
+ return fsync(tar_data->fd);
+}
fsync() should not be called here if --no-sync is used.
+ /* sync the empty blocks as well, since they're after the last file */
+ fsync(tar_data->fd);
Similarly, the fsync call of tar_finish() should not happen when
--no-sync is used.
+ if (format == 'p')
+ stream.walmethod = CreateWalDirectoryMethod(param->xlog, do_sync);
+ else
+ stream.walmethod = CreateWalTarMethod(param->xlog,
compresslevel, do_sync);
LogStreamerMain() exits immediately once it is done, but I think that
we had better be tidy here and clean up the WAL methods that have been
allocated. I am thinking here about a potentially retry method on
failure, though the best shot in this area would be with
ReceiveXlogStream().
Attached is a patch addressing those points.
--
Michael
Вложения
On Mon, Oct 24, 2016 at 7:46 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Sun, Oct 23, 2016 at 10:28 PM, Magnus Hagander <magnus@hagander.net> wrote:
> It also broke the tests and invalidated some documentation. But it was easy
> enough to fix.
>
> I've now applied this, so next time you get to do the merging :P Joking
> aside, please review and let me know if you can spot something I messed up
> in the final merge.
Just had another look at it..
+static int
+tar_fsync(Walfile f)
+{
+ Assert(f != NULL);
+ tar_clear_error();
+
+ /*
+ * Always sync the whole tarfile, because that's all we can do. This makes
+ * no sense on compressed files, so just ignore those.
+ */
+ if (tar_data->compression)
+ return 0;
+
+ return fsync(tar_data->fd);
+}
fsync() should not be called here if --no-sync is used.
+ /* sync the empty blocks as well, since they're after the last file */
+ fsync(tar_data->fd);
Similarly, the fsync call of tar_finish() should not happen when
--no-sync is used.
Yeah, agreed.
+ if (format == 'p')
+ stream.walmethod = CreateWalDirectoryMethod(param->xlog, do_sync);
+ else
+ stream.walmethod = CreateWalTarMethod(param->xlog,
compresslevel, do_sync);
LogStreamerMain() exits immediately once it is done, but I think that
we had better be tidy here and clean up the WAL methods that have been
allocated. I am thinking here about a potentially retry method on
failure, though the best shot in this area would be with
ReceiveXlogStream().
Wouldn't the same be needed in pg_receivexlog.c in that case?
On Tue, Oct 25, 2016 at 7:12 PM, Magnus Hagander <magnus@hagander.net> wrote: > On Mon, Oct 24, 2016 at 7:46 AM, Michael Paquier <michael.paquier@gmail.com> > wrote: >> >> On Sun, Oct 23, 2016 at 10:28 PM, Magnus Hagander <magnus@hagander.net> >> wrote: >> + if (format == 'p') >> + stream.walmethod = CreateWalDirectoryMethod(param->xlog, do_sync); >> + else >> + stream.walmethod = CreateWalTarMethod(param->xlog, >> compresslevel, do_sync); >> LogStreamerMain() exits immediately once it is done, but I think that >> we had better be tidy here and clean up the WAL methods that have been >> allocated. I am thinking here about a potentially retry method on >> failure, though the best shot in this area would be with >> ReceiveXlogStream(). > > Wouldn't the same be needed in pg_receivexlog.c in that case? Oops, missed that. Thanks for the extra checks. Attached is an updated patch. -- Michael
Вложения
On Tue, Oct 25, 2016 at 2:52 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Tue, Oct 25, 2016 at 7:12 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Mon, Oct 24, 2016 at 7:46 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>>
>> On Sun, Oct 23, 2016 at 10:28 PM, Magnus Hagander <magnus@hagander.net>
>> wrote:
>> + if (format == 'p')
>> + stream.walmethod = CreateWalDirectoryMethod(param->xlog, do_sync);
>> + else
>> + stream.walmethod = CreateWalTarMethod(param->xlog,
>> compresslevel, do_sync);
>> LogStreamerMain() exits immediately once it is done, but I think that
>> we had better be tidy here and clean up the WAL methods that have been
>> allocated. I am thinking here about a potentially retry method on
>> failure, though the best shot in this area would be with
>> ReceiveXlogStream().
>
> Wouldn't the same be needed in pg_receivexlog.c in that case?
Oops, missed that. Thanks for the extra checks. Attached is an updated patch.
Thanks, applied and pushed.
On Wed, Oct 26, 2016 at 2:00 AM, Magnus Hagander <magnus@hagander.net> wrote: > Thanks, applied and pushed. Thanks. -- Michael