On Monday, January 29, 2024 7:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jan 29, 2024 at 3:11 PM shveta malik <shveta.malik@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.
Best Regards,
Hou zj