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
