On Wed, Mar 5, 2025 at 3:55 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Tue, Mar 4, 2025 at 8:05 PM Shubham Khanna
> <khannashubham1197@gmail.com> wrote:
> >
> > The attached Patch contains the suggested changes.
> >
>
> Hi Shubham,
>
> Here are few comments for 040_pg_createsubscriber.pl
>
> 1)
> +# Run pg_createsubscriber on node S using '--cleanup-existing-publications'.
> +# --verbose is used twice to show more information.
>
> 1a) /node S/ node S1
>
> 1b) Also, can we keep the comment in the same pattern as it was earlier -
> # Run pg_createsubscriber on node S1. --verbose is used twice
> # to show more information.
> # In passing, also test the --enable-two-phase and
> --cleanup-existing-publications
> # options.
>
Fixed.
> 2)
> +# Reuse P as primary
> +# Set up node S2 as standby linking to node P
> +$node_p->backup('backup_3');
>
> /Reuse P as/ Reuse node P as/
>
Fixed.
> 3)
> +$node_s2->append_conf(
> + 'postgresql.conf', qq[
> + primary_conninfo = '$pconnstr'
> + hot_standby_feedback = on
> + max_logical_replication_workers = 5
> + ]);
>
> Do we need "hot_standby_feedback = on" on node_s2? I think we can remove it.
>
Removed and updated the configurations.
> 4)
> +# Create user-defined publications
> +$node_p->safe_psql($db3, "CREATE PUBLICATION test_pub3 FOR ALL TABLES;");
> +$node_p->safe_psql($db3, "CREATE PUBLICATION test_pub4 FOR ALL TABLES;");
>
> Can create both publications under one safe_psql as -
>
> $node_p->safe_psql($db3, qq[CREATE PUBLICATION test_pub3 FOR ALL TABLES;
> CREATE PUBLICATION test_pub4 FOR ALL TABLES;
> ]);
>
Fixed.
> 5)
> +# Run pg_createsubscriber on node A without using
> +# '--cleanup-existing-publications'.
> +# --verbose is used twice to show more information.
>
> 5a) /node A/node S2/
> 5b) /without using '--cleanup-existing-publications' / without
> '--cleanup-existing-publications' option
>
Fixed.
> 6)
> + ],
> + 'run pg_createsubscriber without --cleanup-existing-publications on node A'
> +);
>
> /node A/node S2/
>
Fixed.
> 7)
> +# Drop the newly created publications
> +$node_p->safe_psql($db3, "DROP PUBLICATION IF EXISTS test_pub3;");
> +$node_p->safe_psql($db3, "DROP PUBLICATION IF EXISTS test_pub4;");
>
> Similar to #4, use single safe_psql to drop both the publications.
> OTOH, do we really need to drop the publications here? I think we can
> remove this part since we're performing cleanup right after.
> ~~~~
>
> --
Fixed.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
