On Monday, January 29, 2024 9:17 PM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
> On Monday, January 29, 2024 7:30 PM Amit Kapila <amit.kapil...@gmail.com>
> wrote:
> >
> > On Mon, Jan 29, 2024 at 3:11 PM shveta malik <shveta.ma...@gmail.com>
> > wrote:
> > >
> > > PFA v71 patch set with above changes.
> > >
> >
> > Few comments on 0001
>
> Thanks for the comments.
>
> > ===================
> > 1.
> > parse_subscription_options()
> > {
> > ...
> > /*
> > * We've been explicitly asked to not connect, that requires some
> > * additional processing.
> > */
> > if (!opts->connect && IsSet(supported_opts, SUBOPT_CONNECT)) {
> >
> > Here, along with other options, we need an explicit check for
> > failover, so that if connect=false and failover=true, the statement
> > should give error. I was expecting the below statement to fail but it passed
> with WARNING.
> > postgres=# create subscription sub2 connection 'dbname=postgres'
> > publication pub2 with(connect=false, failover=true);
> > WARNING: subscription was created, but is not connected
> > HINT: To initiate replication, you must manually create the
> > replication slot, enable the subscription, and refresh the subscription.
> > CREATE SUBSCRIPTION
>
> Added.
>
> >
> > 2.
> > @@ -148,6 +153,10 @@ typedef struct Subscription
> > List *publications; /* List of publication names to subscribe to */
> > char *origin; /* Only publish data originating from the
> > * specified origin */
> > + bool failover; /* True if the associated replication slots
> > + * (i.e. the main slot and the table sync
> > + * slots) in the upstream database are enabled
> > + * to be synchronized to the standbys. */
> > } Subscription;
> >
> > Let's add this new field immediately after "bool runasowner;" as is
> > done for other boolean members. This will help avoid increasing the
> > size of the structure due to alignment when we add any new pointer
> > field in the future. Also, that would be consistent with what we do for
> > other
> new boolean members.
>
> Moved this field as suggested.
>
> Attach the V72-0001 which addressed above comments, other patches will be
> rebased and posted after pushing first patch. Thanks Shveta for helping
> address the comments.
Apart from above comments. The new V72 patch also includes the followings
changes.
1. Moved the test 'altering failover for enabled sub' to the tap-test where most
of the alter-sub behaviors are tested.
2. Rename the tap-test from 050_standby_failover_slots_sync.pl to
040_standby_failover_slots_sync.pl (the big number 050 was used to avoid
conflict with other newly committed tests). And add the test into meson.build
which was missed.
Best Regards,
Hou zj
