From: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
---|---|
To: | vignesh C <vignesh21(at)gmail(dot)com> |
Cc: | Peter Smith <smithpb2250(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, "kuroda(dot)hayato(at)fujitsu(dot)com" <kuroda(dot)hayato(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Handle infinite recursion in logical replication setup |
Date: | 2022-06-20 10:32:13 |
Message-ID: | CAFiTN-svgehftZLJq1pMoRKhFFVYS3avTJ5je=kqnPwuMgdf0g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Jun 20, 2022 at 3:16 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, Jun 20, 2022 at 2:37 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Thu, Jun 16, 2022 at 3:48 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Jun 15, 2022 at 12:09 PM Peter Smith <smithpb2250(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2022-06-20 10:36:47 | Re: pltcl crash on recent macOS |
Previous Message | vignesh C | 2022-06-20 09:46:43 | Re: Handle infinite recursion in logical replication setup |