Re: Handle infinite recursion in logical replication setup

On Wed, Jun 15, 2022 at 12:11 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for patch v20-0003.
>
> ======
>
> 1. Commit message
>
> In case, table t1 has a unique key, it will lead to a unique key
> violation and replication won't proceed.
>
> SUGGESTION
> If table t1 has a unique key, this will lead to a unique key
> violation, and replication won't proceed.

Modified

> ~~~
>
> 2. Commit message
>
> This problem can be solved by using...
>
> SUGGESTION
> This problem can be avoided by using...

Modified

> ~~~
>
> 3. Commit message
>
> step 3: Create a subscription in node1 to subscribe to node2. Use
> 'copy_data = on' so that the existing table data is copied during
> initial sync:
> node1=# CREATE SUBSCRIPTION sub_node1_node2 CONNECTION '<node2 details>'
> node1-# PUBLICATION pub_node2 WITH (copy_data = off, origin = local);
> CREATE SUBSCRIPTION
>
> This is wrong information. The table on node2 has no data, so talking
> about 'copy_data = on' is inappropriate here.

Modified

> ======
>
> 4. Commit message
>
> IMO it might be better to refer to subscription/publication/table "on"
> nodeXXX, instead of saying "in" nodeXXX.
>
> 4a.
> "the publication tables were also subscribing data in the publisher
> from other publishers." -> "the publication tables were also
> subscribing from other publishers.

Modified

> 4b.
> "After the subscription is created in node2" -> "After the
> subscription is created on node2"

Modified

> 4c.
> "step 3: Create a subscription in node1 to subscribe to node2." ->
> "step 3: Create a subscription on node1 to subscribe to node2."

Modified

> 4d.
> "step 4: Create a subscription in node2 to subscribe to node1." ->
> "step 4: Create a subscription on node2 to subscribe to node1."

Modified

> ======
>
> 5. doc/src/sgml/ref/create_subscription.sgml
>
> @@ -383,6 +397,15 @@ CREATE SUBSCRIPTION <replaceable
> class="parameter">subscription_name</replaceabl
>     can have non-existent publications.
>    </para>
>
> +  <para>
> +   If the subscription is created with <literal>origin = local</literal> and
> +   <literal>copy_data = on</literal>, it will check if the publisher tables are
> +   being subscribed to any other publisher and, if so, then 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>
>
> Perhaps it is better here to say 'copy_data = true' instead of
> 'copy_data = on', simply because the value 'true' was mentioned
> earlier on this page (but this would be the first mention of 'on').

Modified

> ======
>
> 6. src/backend/commands/subscriptioncmds.c - check_pub_table_subscribed
>
> + errhint("Use CREATE/ALTER SUBSCRIPTION with copy_data = off or force."));
>
> Saying "off or force" is not consistent with the other message wording
> in this patch, which used "/" for multiple enums.
> (e.g. "connect = false", "copy_data = true/force").
>
> So perhaps this errhint should be worded similarly:
> "Use CREATE/ALTER SUBSCRIPTION with copy_data = off/force."

Modified

Thanks for the comments, the v21 patch attached at [1] has the changes
for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm3%2B6cey0rcDft1ZUCjSUtLDM0xmU_Q%2BYhcsBrqe1RH8%3Dw%40mail.gmail.com

Regards,
Vignesh