On Mon, Oct 17, 2016 at 7:37 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
> On Sat, Oct 15, 2016 at 8:51 AM, Magnus Hagander <mag...@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.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
