Thanks for working on my review comments. I'll take a look at the new
changes and let you know my comments, if any. I didn't get a chance to
check it out today as I was busy reviewing some other patches, but
I'll definitely take a look at the new patch in a day or so and let
you know my feedback.
--
With Regards,
Ashutosh Sharma.
On Sun, Feb 13, 2022 at 7:34 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, Feb 11, 2022 at 7:14 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > I have spent little time looking at the latest patch. The patch looks
> > to be in good shape as it has already been reviewed by many people
> > here, although I did get some comments. Please take a look and let me
> > know your thoughts.
> >
> >
> > + /* Try to connect to the publisher. */
> > + wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> > + if (!wrconn)
> > + ereport(ERROR,
> > + (errmsg("could not connect to the publisher: %s", err)));
> >
> > I think it would be good to also include the errcode
> > (ERRCODE_CONNECTION_FAILURE) here?
>
> Modified
>
> > --
> >
> > @@ -514,6 +671,8 @@ CreateSubscription(ParseState *pstate,
> > CreateSubscriptionStmt *stmt,
> >
> > PG_TRY();
> > {
> > + check_publications(wrconn, publications, opts.validate_publication);
> > +
> >
> >
> > Instead of passing the opts.validate_publication argument to
> > check_publication function, why can't we first check if this option is
> > set or not and accordingly call check_publication function? For other
> > options I see that it has been done in the similar way for e.g. check
> > for opts.connect or opts.refresh or opts.enabled etc.
>
> Modified
>
> > --
> >
> > Above comment also applies for:
> >
> > @@ -968,6 +1130,8 @@ AlterSubscription(ParseState *pstate,
> > AlterSubscriptionStmt *stmt,
> > replaces[Anum_pg_subscription_subpublications - 1] = true;
> >
> > update_tuple = true;
> > + connect_and_check_pubs(sub, stmt->publication,
> > + opts.validate_publication);
> >
>
> Modified
>
> > --
> >
> > + <para>
> > + When true, the command verifies if all the specified publications
> > + that are being subscribed to are present in the publisher and throws
> > + an error if any of the publication doesn't exist. The default is
> > + <literal>false</literal>.
> >
> > publication -> publications (in the 4th line : throw an error if any
> > of the publication doesn't exist)
> >
> > This applies for both CREATE and ALTER subscription commands.
>
> Modified
>
> Thanks for the comments, the attached v14 patch has the changes for the same.
>
> Regard,s
> Vignesh
