Re: [HACKERS] Make pg_basebackup -x stream the default

On Sun, Jan 1, 2017 at 12:47 AM, Michael Paquier <michael.paquier@gmail.com> wrote:

On Sat, Dec 31, 2016 at 9:24 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Tue, Dec 20, 2016 at 11:53 PM, Michael Paquier
> <michael.paquier@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.