Re: Handle infinite recursion in logical replication setup - Mailing list pgsql-hackers

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.

~~~

2. Commit message

This problem can be solved by using...

SUGGESTION
This problem can be avoided by using...

~~~

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.

======

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.

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

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

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

======

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').

======

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."

------
Kind Regards,
Peter Smith.
Fujitsu Australia