Re: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided. - Mailing list pgsql-hackers

On Tue, 11 Feb 2025 at 15:46, Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Tue, Feb 11, 2025 at 6:30 AM Peter Smith <smithpb2250@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
>

Hi Shubham,

I reviewed v6 patch, here are some of my comments:

1. Option 'P' is for "publisher-server".
            case 'P':
+               if (opt.all_databases)
+               {
+                   pg_log_error("--publication cannot be used with
--all-databases");
+                   exit(1);
+               }

So, if we provide the "--all-databases" option and then the
"--publisher-server" option. Then the command pg_createsubscriber is
failing, which is wrong.

2. In case we provide "--all-database" option and then "--publication"
option, the pg_createsubscriber command is running. I think the above
condition should be added at "case 2:" instead of "case 'P':"

Thanks and Regards,
Shlok Kyal