Thread: Make pg_basebackup -x stream the default

On 11/8/16 12:45 PM, Magnus Hagander wrote:
> Per some discussions with a number of different people at pgconfeu, here
> is a patch that changes the default mode of pg_basebackup to be
> streaming the wal, as this is what most users would want -- and those
> that don't want it have to make other changes as well. Doing the "most
> safe" thing by default is a good idea.
>
> The new option "-x none" is provided to turn this off and get the
> previous behavior back.

I would have expected that the "stream" method would become the default.
 Just a short while ago it was proposed to remove the "fetch" method
altogether, and it was only kept because of some niche use cases.  I
don't think "fetch" is the most safe method, because it requires
wal_keep_segments to be configured.

On 12/15/2016 10:23 AM, Peter Eisentraut wrote:

On 11/8/16 12:45 PM, Magnus Hagander wrote:

Per some discussions with a number of different people at pgconfeu, here
is a patch that changes the default mode of pg_basebackup to be
streaming the wal, as this is what most users would want -- and those
that don't want it have to make other changes as well. Doing the "most
safe" thing by default is a good idea.

The new option "-x none" is provided to turn this off and get the
previous behavior back.

I would have expected that the "stream" method would become the default.
 Just a short while ago it was proposed to remove the "fetch" method
altogether, and it was only kept because of some niche use cases.  I
don't think "fetch" is the most safe method, because it requires
wal_keep_segments to be configured.

IMO, if you are using fetch just using archive_command. Let's rip it out of pg_basebackup or at least deprecate it.

Usability review

============

Patch sounds like a good idea and does what it supposed to do. /me in DBA hat will be happy to have it.

However, it makes '-x' parameter a bit confusing/surprising: specifying it will be equivalent to '-X fetch' which is surprising regression from the new default.

One comment about docs:

         Includes the required transaction log files (WAL files) in the

         backup. This will include all transaction logs generated during

-        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 option <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>

        <para>

I suggest "method <literal>none</literal>" instead of "option <literal>none</literal>". I found the word "option" confusing in that sentence.

On Fri, Dec 16, 2016 at 11:36 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Thu, Dec 15, 2016 at 12:37 AM, Vladimir Rusinov <vrusinov@google.com>
> wrote:
>>
>> Usability review
>>
>> ============
>>
>>
>> Patch sounds like a good idea and does what it supposed to do. /me in DBA
>> hat will be happy to have it.
>>
>> However, it makes '-x' parameter a bit confusing/surprising: specifying it
>> will be equivalent to '-X fetch' which is surprising regression from the new
>> default.
>
>
> This seems like a good idea, really.
>
> Given that we already break a number of other things around backups and
> replication in this release, it seems like a good time.
>
> I definitely think removing it is what we should do -- let's not redefine it
> to mean streaming, let's just get rid of -x altogether, and have people use
> -X streaming|fetch|none.
>
> What do others feel about this?

+1 to drop -x option. That's less confusing.

Attached is an updated patch that does this. As a bonus it simplifies the code a bit. I also fixed an error message that I missed updating in the previous patch. 

On Mon, Dec 19, 2016 at 7:51 PM, Vladimir Rusinov <vrusinov@google.com> wrote:
>
> On Sat, Dec 17, 2016 at 2:37 PM, Magnus Hagander <magnus@hagander.net>
> wrote:
>>
>> Attached is an updated patch that does this. As a bonus it simplifies the
>> code a bit. I also fixed an error message that I missed updating in the
>> previous patch.
>
>
> looks good to me.

Yep, basically looks good to me. Here are some small comments.

> while ((c = getopt_long(argc, argv, "D:F:r:RT:xX:l:nNzZ:d:c:h:p:U:s:S:wWvP",
> long_options, &option_index)) != -1)

'x' should be removed from the above code.

> To create a backup of a single-tablespace local database and compress this with bzip2:
>
> $ pg_basebackup -D - -Ft | bzip2 > backup.tar.bz2

The above example in the docs needs to be modified. For example,
add "-X fetch" into the above command so that pg_basebackup
should not exit with an error.

> The server must also be configured with max_wal_senders set high
> enough to leave at least one session available for the backup.

I think that it's better to explain explicitly here that max_wal_senders
should be higher than one by default.

On Tue, Dec 20, 2016 at 10:38 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Mon, Dec 19, 2016 at 7:51 PM, Vladimir Rusinov <vrusinov@google.com> wrote:
>> The server must also be configured with max_wal_senders set high
>> enough to leave at least one session available for the backup.
>
> I think that it's better to explain explicitly here that max_wal_senders
> should be higher than one by default.

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";
}

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.

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.

On Sat, Jan 7, 2017 at 12:04 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Wed, Jan 4, 2017 at 10:43 AM, Magnus Hagander <magnus@hagander.net>
> wrote:
>> 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?

Yes, that's it. I have looked at the patch in details and that looks
correct to me.