Re: Handle infinite recursion in logical replication setup - Mailing list pgsql-hackers
From | vignesh C |
---|---|
Subject | Re: Handle infinite recursion in logical replication setup |
Date | |
Msg-id | CALDaNm0APLRcyvDAdUhOkaTqhXT4a=50g-it2tHrTjE4ebVXjQ@mail.gmail.com Whole thread Raw |
In response to | Re: Handle infinite recursion in logical replication setup (Dilip Kumar <dilipbalaut@gmail.com>) |
Responses |
Re: Handle infinite recursion in logical replication setup
Re: Handle infinite recursion in logical replication setup |
List | pgsql-hackers |
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: + /* + * Throw an error if the publisher has subscribed to the same table + * from some other publisher. We cannot differentiate between the + * local and non-local data that is present in the HEAP during the + * initial sync. Identification of local data can be done only from + * the WAL by using the origin id. XXX: For simplicity, we don't check + * whether the table has any data or not. If the table doesn't have + * any data then we don't need to distinguish between local and + * non-local data so we can avoid throwing error in that case. + */ + if (!slot_attisnull(slot, 3)) + ereport(ERROR, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("table:%s.%s might have replicated data in the publisher", + nspname, relname), + errdetail("CREATE/ALTER SUBSCRIPTION with origin = local and copy_data = on is not allowed when the publisher might have replicated data."), + errhint("Use CREATE/ALTER SUBSCRIPTION with copy_data = off/force.")); [1] - https://wiki.postgresql.org/wiki/PgCon_2022_Developer_Unconference#Logical_Replication_Origin_Filtering_and_Consistency Regards, Vignesh
pgsql-hackers by date: