Re: Handle infinite recursion in logical replication setup

On Mon, Jul 25, 2022 at 12:58 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Firstly, I have some (case-sensitivity) questions about the previous
> patch which was already pushed [1].
>
> Q1. create_subscription docs
>
> I did not understand why the docs refer to slot_name = NONE, yet the
> newly added option says origin = none/any. I think that saying origin
> = NONE/ANY would be more consistent with the existing usage of NONE in
> this documentation.
>
> ~~~
>
> Q2. parse_subscription_options
>
> Similarly, in the code (parse_subscription_options), I did not
> understand why the checks for special name values are implemented
> differently:
>
> The new 'origin' code is using pg_strcmpcase to check special values
> (none/any), and the old 'slot_name' code uses case-sensitive strcmp to
> check the special value (none).
>
> FWIW, here I thought the new origin code is the correct one.
>
> ======
>
> Now, here are some review comments for the patch v38-0001:
>
> 1. src/backend/commands/subscriptioncmds.c - check_pub_table_subscribed
>
> @@ -1781,6 +1858,121 @@ AlterSubscriptionOwner_oid(Oid subid, Oid newOwnerId)
>   table_close(rel, RowExclusiveLock);
>  }
>
> +/*
> + * 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 for CREATE SUBSCRIPTION and
> + * ALTER SUBSCRIPTION ... REFRESH statements to avoid replicating remote data
> + * from the publisher.
> + *
> + * This check need not be performed on the tables that are already added as
> + * incremental sync for such tables will happen through WAL and the origin of
> + * the data can be identified from the WAL records.
> + *
> + * subrel_local_oids contains the list of relation oids that are already
> + * present on the subscriber.
> + */
> +static void
> +check_pub_table_subscribed(WalReceiverConn *wrconn, List *publications,
> +    CopyData copydata, char *origin,
> +    Oid *subrel_local_oids, int subrel_count)
>
> 1a.
> "copydata is ON" --> "copy_data = on" (because the comment is talking
> about the CREATE/ALTER statements, so it seemed a bit confusing to
> refer to the copydata function param instead of the copy_data
> subscription parameter)

Modified

> 1b.
> "the origin is local" ?? But, "local" was the old special name value.
> Now it is "none", so I think this part needs minor rewording.

Modified

>
> ~~~
>
> 2.
>
> + if (copydata != COPY_DATA_ON || !origin ||
> + (pg_strcasecmp(origin, "none") != 0))
> + return;
>
> Should this be using the constant LOGICALREP_ORIGIN_NONE?

Modified

> ~~~
>
> 3.
>
> + /*
> + * Throw an error if the publisher has subscribed to the same table
> + * from some other publisher. We cannot differentiate between the
> + * origin and non-origin data that is present in the HEAP during the
> + * initial sync. Identification of non-origin 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 an error in that case.
> + */
>
> 3a.
> When the special origin value changed from "local" to "none" this
> comment's first part seems to have got a bit lost in translation.

Modified

> SUGGESTION:
> Throw an error if the publisher has subscribed to the same table from
> some other publisher. We cannot know the origin of data during the
> initial sync. Data origins can be found only from the WAL by looking
> at the origin id.

Modified

> 3b.
> I think referring to "local and non-local" data in the XXX part of
> this comment also needs some minor rewording now that "local" is not a
> special origin name anymore.

Modified

Thanks for the comments, the v39 patch shared at [1] has the changes
for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm2POATc_jwQ-8MBJgGCVZGdUNhnTv8zkBuGzLaY03dM%3DA%40mail.gmail.com

Regards,
Vignesh