On Mon, Jun 20, 2022 at 3:16 PM vignesh C <vignes...@gmail.com> wrote: > > On Mon, Jun 20, 2022 at 2:37 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Thu, Jun 16, 2022 at 3:48 PM vignesh C <vignes...@gmail.com> wrote: > > > > > > On Wed, Jun 15, 2022 at 12:09 PM Peter Smith <smithpb2...@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
