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.