From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | vignesh C <vignesh21(at)gmail(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, "kuroda(dot)hayato(at)fujitsu(dot)com" <kuroda(dot)hayato(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Handle infinite recursion in logical replication setup |
Date: | 2022-06-15 06:41:21 |
Message-ID: | CAHut+PvQt9h=zOs6SYHOdUh=CvDYp0bqZzZc0hNbCYt_dEdx=A@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Smith | 2022-06-15 06:43:35 | Re: Handle infinite recursion in logical replication setup |
Previous Message | Peter Smith | 2022-06-15 06:39:29 | Re: Handle infinite recursion in logical replication setup |