Re: Handle infinite recursion in logical replication setup

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:
+ /*
+ * 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