Re: Make pg_basebackup -x stream the default

On Wed, Jan 4, 2017 at 10:43 AM, Magnus Hagander <magnus(at)hagander(dot)net>
wrote:

>
>
> On Sun, Jan 1, 2017 at 12:47 AM, Michael Paquier <
> michael(dot)paquier(at)gmail(dot)com> wrote:
>
>> On Sat, Dec 31, 2016 at 9:24 PM, Magnus Hagander <magnus(at)hagander(dot)net>
>> wrote:
>> > On Tue, Dec 20, 2016 at 11:53 PM, Michael Paquier
>> > <michael(dot)paquier(at)gmail(dot)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.
>
>
Here's a patch that does this. Does this match what you were thinking?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/