On Mon, Jun 20, 2022 at 3:16 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, Jun 20, 2022 at 2:37 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Thu, Jun 16, 2022 at 3:48 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Wed, Jun 15, 2022 at 12:09 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > > >
> >
> > > Thanks for the comments, the attached v21 patch has the changes for the same.
> >
> > I have done some basic review of v21 and I have a few comments,
> >
> > 1.
> > +/*
> > + * The subscription will request the publisher to only send changes that
> > + * originated locally.
> > + */
> > +#define LOGICALREP_ORIGIN_LOCAL "local"
> > +
> > +/*
> > + * The subscription will request the publisher to send any changes regardless
> > + * of their origin
> > + */
> > +#define LOGICALREP_ORIGIN_ANY "any"
> >
> > Are we planning to extend this to more options or are we planning to
> > support the actual origin name here? If not then why isn't it just
> > bool? I think the comments and the patch commit message should
> > explain the details behind it if it has been already discussed and
> > concluded.
>
> Currently we only support local and any. But this was designed to
> accept string instead of boolean type, so that it can be extended
> later to support filtering of origin names specified by the user in
> the later versions. The same was also discussed in pg unconference as
> mentioned in [1]. I will add it to the commit message and a comment
> for the same in the next version.
>
> > 2.
> > +/*
> > + * Check and throw an error if the publisher has subscribed to the same table
> > + * from some other publisher. This check is required only if copydata is ON and
> > + * the origin is local.
> > + */
> >
> > I think it should also explain why this combination is not allowed and
> > if it is already explained in code
> > then this code can add comments to refer to that part of the code.
>
> In the same function, the reason for this is mentioned detailly just
> above the place where error is thrown. I think that should be enough.
> Have a look and let me know if that is not sufficient:
I think that should be sufficient, thanks.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com