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 <mag...@hagander.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@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
smime.p7s
Description: S/MIME Cryptographic Signature
