Re: Make pg_basebackup -x stream the default

From: Vladimir Rusinov <vrusinov(at)google(dot)com>
To: Magnus Hagander <magnus(at)hagander(dot)net>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Make pg_basebackup -x stream the default
Date: 2016-12-14 23:37:27
Message-ID: CAE1wr-xyM+9N0iOM1rWdWO5bE-FMYAQCgk65oQqMXMqt2V-s-w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Summary

=======

Thank you for submission! I think it needs a bit more work to be even
better.

Please deal with '-x' argument and with wording in documentation.

I'll set status to 'waiting on author' now.

Submission review

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

Patch is in correct format.

Patch applies cleanly on HEAD.

Tests pass. There seems to be sufficient amount of tests to cover a change.

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.

I think we need to either change -x to be equivalent to '-X streaming' or
just get rid of it altogether.

Feature test

===========

I've tested the change manually by doing pg_basebackup with -X none, -X
streaming and -X fetch and their shorthand variants, each with
max_wal_senders sent to 1 and 2.

I've got all the expected results/failures (e.g. -X streaming fails when
max_wal_senders set to 1).

Regression tests pass.

Coding review

===========

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.

--
Vladimir Rusinov
Storage SRE, Google Ireland

Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland
Registered in Dublin, Ireland
Registration Number: 368047

On Tue, Nov 8, 2016 at 5:45 PM, Magnus Hagander <magnus(at)hagander(dot)net> 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 will add this to the next (January) commitfest.
>
> //Magnus
>
>
>
> --
> Magnus Hagander
> Me: http://www.hagander.net/
> Work: http://www.redpill-linpro.com/
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2016-12-14 23:51:31 Re: [PATCH] Rename pg_switch_xlog to pg_switch_wal
Previous Message Peter Eisentraut 2016-12-14 23:31:38 Re: [PATCH] Remove trailing whitespaces from documentation