Hi Shubham.
Responding with a blanket statement "Fixed the given comments" makes
it difficult to be sure what was done when I come to confirm the
changes. Often embedded questions go unanswered, and if changes are
*not* made, then I can't tell if there was a good reason for rejection
or was the comment just overlooked. Please can you treat the itemised
feedback as a checklist and reply individually to each item. Even just
saying "Fixed" is useful because then I can trust the item was seen
and addressed.
e.g. From previous review [1]
#7. Not fixed. The same docs issue still exists for --publication and
for --subscription.
#13. Unanswered question "How are tests expecting this even passing?".
Was a reason identified? IOW, how can we be sure the latest tests
don't have a similar problem?
//////
Some more review comments for the patch v5-0001.
======
doc/src/sgml/ref/pg_createsubscriber.sgml
Synopsis
1.
pg_createsubscriber [option...] { -a | --all-databases } { -d |
--database }dbname { -D | --pgdata }datadir { -P | --publisher-server
}connstr
This looks misleading because '-all-database' and '--database' cannot
be combined.
I think it will be better to have 2 completely different synopsis like
I had originally suggested [2, comment #5]. E.g. just add a whole
seperate <cmdsynopsis> block adjacent to the other one in the SGML:
------
<cmdsynopsis>
<command>pg_createsubscriber</command>
<arg rep="repeat"><replaceable>option</replaceable></arg>
<group choice="plain">
<group choice="req">
<arg choice="plain"><option>-a</option></arg>
<arg choice="plain"><option>--all-databases</option></arg>
</group>
<group choice="req">
<arg choice="plain"><option>-D</option> </arg>
<arg choice="plain"><option>--pgdata</option></arg>
</group>
<replaceable>datadir</replaceable>
<group choice="req">
<arg choice="plain"><option>-P</option></arg>
<arg choice="plain"><option>--publisher-server</option></arg>
</group>
<replaceable>connstr</replaceable>
</group>
</cmdsynopsis>
------
~~~
2. --all-databases
+ <para>
+ <option>--all-databases</option> cannot be used with
+ <option>--database</option>, <option>--publication</option>,
+ <option>--replication-slot</option> or <option>--subscription</option>.
+ </para>
If you want consistent wording with the other places in this patch,
then this should just be the last sentence of the previous paragraph
and be worded like: "Cannot be used together with..."
~~~
3. --publication
- a generated name is assigned to the publication name.
+ a generated name is assigned to the publication name. Cannot be used
+ together with <option>-a</option>.
Previously reported. Claimed fixed -a to --all-databases. Not fixed.
~~~
4. --subscription
- a generated name is assigned to the subscription name.
+ a generated name is assigned to the subscription name. Cannot be used
+ together with <option>-a</option>.
(same as the previous review comment).
Previously reported. Claimed fixed -a to --all-databases. Not fixed.
======
src/bin/pg_basebackup/pg_createsubscriber.c
5.
+ bool all_databases; /* fetch and specify all databases */
Comment wording. "and specified" (??). Maybe just "--all-databases
option was specified"
======
[1]
https://www.postgresql.org/message-id/CAHut%2BPuYmTyi6kPFnJDTvD%3DaLHd0kyX4VG6iDQVEk-ixov1AwA%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPuyBsOJTSygus2-yp60sw_phwYQ-JyC%2BU6fCBMos9x2LA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
