Re: Handle infinite recursion in logical replication setup

On Fri, Jul 29, 2022 at 8:31 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some comments for the patch v40-0001:
>
> ======
>
> 1. Commit message
>
> It might be better to always use 'copy_data = true' in favour of
> 'copy_data = on' just for consistency with all the docs and the error
> messages.
>
> ======

Modified

> 2. doc/src/sgml/ref/create_subscription.sgml
>
> @@ -386,6 +401,15 @@ CREATE SUBSCRIPTION <replaceable
> class="parameter">subscription_name</replaceabl
>     can have non-existent publications.
>    </para>
>
> +  <para>
> +   If the subscription is created with <literal>origin = NONE</literal> and
> +   <literal>copy_data = true</literal>, it will check if the publisher has
> +   subscribed to the same table from other publishers and, if so, throw an
> +   error to prevent possible non-local data from being copied. The user can
> +   override this check and continue with the copy operation by specifying
> +   <literal>copy_data = force</literal>.
> +  </para>
>
> 2a.
> It is interesting that you changed the note to say origin = NONE.
> Personally, I prefer it written as you did, but I think maybe this
> change does not belong in this patch. The suggestion for changing from
> "none" to NONE is being discussed elsewhere in this thread and
> probably all such changes should be done together (if at all) as a
> separate patch. Until then I think this patch 0001 should just stay
> consistent with whatever is already pushed on HEAD.

Modified

> 2b.
> "possible no-local data". Maybe the terminology "local/non-local" is a
> hangover from back when the subscription parameter was called local
> instead of origin. I'm not sure if you want to change this or not, and
> anyway I didn't have any better suggestions – so this comment is just
> to bring it to your attention.
>
> ======

Modified

> 3. src/backend/commands/subscriptioncmds.c - DefGetCopyData
>
> + /*
> + * The set of strings accepted here should match up with the
> + * grammar's opt_boolean_or_string production.
> + */
> + if (pg_strcasecmp(sval, "false") == 0 ||
> + pg_strcasecmp(sval, "off") == 0)
> + return COPY_DATA_OFF;
> + if (pg_strcasecmp(sval, "true") == 0 ||
> + pg_strcasecmp(sval, "on") == 0)
> + return COPY_DATA_ON;
> + if (pg_strcasecmp(sval, "force") == 0)
> + return COPY_DATA_FORCE;
>
> I understand the intention of the comment, but it is not strictly
> correct to say "should match up" because "force" is a new value.
> Perhaps the comment should be as suggested below.
>
> SUGGESTION
> The set of strings accepted here must include all those accepted by
> the grammar's opt_boolean_or_string production.
>
> ~~

Modified

>
> 4. src/backend/commands/subscriptioncmds.c - check_pub_table_subscribed
>
> @@ -1781,6 +1858,122 @@ 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 "copy_data = on"
> + * and "origin = NONE" for CREATE SUBSCRIPTION and
> + * ALTER SUBSCRIPTION ... REFRESH statements to avoid the publisher from
> + * replicating data that has an origin.
> + *
> + * 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)
>
> 4a.
> "copy_data = on" -> "copy_data = true" (for consistency with the docs
> and the error messages)

Modified

> 4b.
> The same NONE/none review comment from #2a applies here too. Probably
> it should be written as none for now unless/until *everything* changes
> to NONE.

Modified

> 4c.
> "to avoid the publisher from replicating data that has an origin." ->
> "to avoid replicating data that has an origin."

Modified

> 4d.
> + * 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.
>
> SUGGESTION (maybe?)
> This check need not be performed on the tables that are already added
> because incremental sync for those tables will happen through WAL and
> the origin of the data can be identified from the WAL records.
>
> ======

Modified

>
> 5. src/test/subscription/t/030_origin.pl
>
> + "Refresh publication when the publisher has subscribed for the new table"
>
> SUGGESTION (Just to mention origin = none somehow. Maybe you can
> reword it better than this)
> Refresh publication when the publisher has subscribed for the new
> table, but the subscriber-side wants origin=none

Modified

Thanks for the comments, the attached v41 patch has the changes for the same.

Regards,
Vignesh