Re: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided.

On Tue, Feb 11, 2025 at 6:30 AM Peter Smith <smithpb2250(at)gmail(dot)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.