On Sun, Jan 1, 2017 at 12:47 AM, Michael Paquier <michael.paqu...@gmail.com>
wrote:
> On Sat, Dec 31, 2016 at 9:24 PM, Magnus Hagander <mag...@hagander.net>
> wrote:
> > On Tue, Dec 20, 2016 at 11:53 PM, Michael Paquier
> > <michael.paqu...@gmail.com> wrote:
> >> Recovery tests are broken by this patch, the backup() method in
> >> PostgresNode.pm uses pg_basebackup -x:
> >> sub backup
> >> {
> >> my ($self, $backup_name) = @_;
> >> my $backup_path = $self->backup_dir . '/' . $backup_name;
> >> my $port = $self->port;
> >> my $name = $self->name;
> >>
> >> print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
> >> TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p',
> >> $port,
> >> '-x', '--no-sync');
> >> print "# Backup finished\n";
> >> }
> >
> >
> > Oh bleh. That's what I get for just running the tests for pg_basebackup
> > itself.
> >
> > I removed it completely in this patch, making it use streaming. Or is
> there
> > a particular reason we want it to use fetch, that I'm not aware of?
>
> Not really. Both should be equivalent in the current tests. It may
> actually be a good idea to add an argument in PostgresNode->backup to
> define the WAL fetching method.
>
> > Attached is a new patch with this fix, and those suggested by Fujii as
> well.
>
> - the backup. If this option is specified, it is possible to start
> - a postmaster directly in the extracted directory without the need
> - to consult the log archive, thus making this a completely
> standalone
> - backup.
> + the backup. Unless the method <literal>none</literal> is
> specified,
> + it is possible to start a postmaster directly in the extracted
> + directory without the need to consult the log archive, thus
> + making this a completely standalone backup.
> </para>
> I find a bit weird to mention a method value in a paragraph before
> listing them. Perhaps it should be a separate paragraph after the list
> of methods available?
>
> -static bool includewal = false;
> -static bool streamwal = false;
> +static bool includewal = true;
> +static bool streamwal = true;
> Two booleans are used to define 3 states. It may be cleaner to use an
> enum instead. The important point is that streaming WAL is enabled
> only if "stream" method is used.
>
> Other than that the patch looks good to me. Tests pass.
>
Meh, just as I was going to respond "committed" I noticed this second round
of review comments. Apologies, pushed without that.
I agree on the change with includewal/streamwal. That was already the case
in the existing code of course, but that doesn't mean it couldn't be made
better. I'll take a look at doing that as a separate patch.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
