On Fri, Mar 14, 2025 at 5:43 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> Hi Shubham,
>
> Here are a few comments for the v12 patch.
>
> doc/src/sgml/ref/pg_createsubscriber.sgml :
>
> 1.
> + <para>
> + For all source server non-template databases create subscriptions for
> + create subscriptions for databases with the same names on the
> + target server.
>
> is “create subscriptions for” repeated? The sentence doesn’t make sense.
>
Fixed.
> 2. Typo
> + switches. This option cannot be together with <option>--all</option>.
>
> /cannot be together/cannot be used together/
> ~~~
>
Fixed.
> 3. src/bin/pg_basebackup/pg_createsubscriber.c :
> + /* Establish a connection to the PostgreSQL server */
> + conn = connect_database(opt->pub_conninfo_str, true);
>
> I think comment will be more clear if say “ Establish a connection to
> the primary server */ or “source server”
> ~~~
>
Fixed.
> src/bin/pg_basebackup/t/042_all_option.pl :
>
> 4. As per Ashutosh's comment at [1], tests need to be added to verify
> logical replication behavior after using the --all option.
> ~~~~
>
> Please refer to the attached top-up fix patch, which includes the
> above changes along with some cosmetic fixes in the test file
> 042_all_option.pl.
>
> [1] https://www.postgresql.org/message-id/CAExHW5uJHYAge99oS_iPfGWwZ_eCr2xFCNnifQkGs2GXeMQKGQ%40mail.gmail.com
>
> --
Fixed.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
