On Tue, Feb 11, 2025 at 6:30 AM Peter Smith <smithpb2...@gmail.com> wrote: > > 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. >
I appreciate the feedback. I will ensure that each item is addressed
individually.
> e.g. From previous review [1]
> #7. Not fixed. The same docs issue still exists for --publication and
> for --subscription.
Fixed this issue in the attached patch.
> #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?
>
In the v4-0001 patch [1], the tests were mistakenly using
'command_fails' instead of 'command_fails_like' to verify failed test
cases. Since 'command_fails' only checks for a non-zero exit code and
not specific error messages, the tests were passing even when the
expected errors were not being triggered correctly.
To address this, I have updated the patch to use 'command_fails_like',
ensuring that the test cases now explicitly verify the correct failure
messages.
> //////
>
> 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>
> ------
>
> ~~~
>
Fixed.
> 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..."
>
> ~~~
>
Fixed.
> 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.
>
> ~~~
>
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.
>
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"
>
> ======
Fixed.
The attached Patch contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BDOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
v6-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patch
Description: Binary data
