Re: Handle infinite recursion in logical replication setup

Here are some review comments for patch v20-0004.

======

1. General

I thought that it is better to refer to the
subscription/publications/table "on" the node, rather than "in" the
node. Most of the review comments below are related to this point.

======

2. Commit message

a) Creating a two-node bidirectional replication when there is no data
in both nodes.
b) Adding a new node when there is no data in any of the nodes.
c) Adding a new node when data is present in the existing nodes.

"in both nodes" -> "on both nodes"
"in any of the nodes" -> "on any of the nodes"
"in the existing nodes" -> "on the existing nodes"

======

3. doc/src/sgml/logical-replication.sgml - Setting bidirectional
replication between two nodes

3a.
+ <para>
+ The following steps demonstrate how to create a two-node bidirectional
+ replication when there is no table data present in both nodes
+ <literal>node1</literal> and <literal>node2</literal>:
+ </para>
-> "on both nodes"

3b.
+ Create a publication in <literal>node1</literal>:
-> "on"

3c.
+ Create a publication in <literal>node2</literal>:
-> "on"

3d.
+ <para>
+ Lock the table <literal>t1</literal> in <literal>node1</literal> and
+ <literal>node2</literal> in <literal>EXCLUSIVE</literal> mode until the
+ setup is completed.
+ </para>
-> "on <literal>node1</literal>"

3e.
+ Create a subscription in <literal>node2</literal> to subscribe to
-> "on"

3f.
+ Create a subscription in <literal>node1</literal> to subscribe to
+ <literal>node2</literal>:
-> "on"

~~~

4. doc/src/sgml/logical-replication.sgml - Adding a new node when
there is no data in any of the nodes

4a.
+ <title>Adding a new node when there is no data in any of the nodes</title>
SUGGESTION
Adding a new node when there is no table data on any of the nodes

4b.
+ <para>
+ The following steps demonstrate adding a new node <literal>node3</literal>
+ to the existing <literal>node1</literal> and <literal>node2</literal> when
+ there is no <literal>t1</literal> data in any of the nodes. This requires
+ creating subscriptions in <literal>node1</literal> and
+ <literal>node2</literal> to replicate the data from
+ <literal>node3</literal> and creating subscriptions in
+ <literal>node3</literal> to replicate data from <literal>node1</literal>
+ and <literal>node2</literal>. Note: These steps assume that the
+ bidirectional logical replication between <literal>node1</literal> and
+ <literal>node2</literal> is already completed.
+ </para>

"data in any of the nodes" -> "data on any of the nodes"
"creating subscriptions in <literal>node1</literal>" -> "creating
subscriptions on <literal>node1</literal>"
"creating subscriptions in <literal>node3</literal>" -> "creating
subscriptions on <literal>node3</literal>"

4c.
+ Create a publication in <literal>node3</literal>:
-> "on"

4d.
+ Lock table <literal>t1</literal> in all the nodes
-> "on"

4e.
+ Create a subscription in <literal>node1</literal> to subscribe to
+ <literal>node3</literal>:
-> "on"

4f.
+ Create a subscription in <literal>node2</literal> to subscribe to
+ <literal>node3</literal>:
-> "on"

4g.
+ Create a subscription in <literal>node3</literal> to subscribe to
+ <literal>node1</literal>:
-> "on"

4h.
+ Create a subscription in <literal>node3</literal> to subscribe to
+ <literal>node2</literal>:

4i.
+ <literal>node3</literal>. Incremental changes made in any node will be
+ replicated to the other two nodes.
"in any node" -> "on any node"

~~~

5. doc/src/sgml/logical-replication.sgml - Adding a new node when data
is present in the existing nodes

5a.
+ <title>Adding a new node when data is present in the existing nodes</title>
SUGGESTION
Adding a new node when table data is present on the existing nodes

5b.
+ during initial data synchronization. Note: These steps assume that the
+ bidirectional logical replication between <literal>node1</literal> and
+ <literal>node2</literal> is already completed, and the pre-existing data
+ in table <literal>t1</literal> is already synchronized in both those
+ nodes.
+ </para>
"in both those nodes" -> "on both those nodes"

5c.
+ Create a publication in <literal>node3</literal>
-> "on"

5d.
+ Lock table <literal>t1</literal> in <literal>node2</literal> and
-> "on"

5e.
+ Create a subscription in <literal>node1</literal> to subscribe to
+ <literal>node3</literal>:
-> "on"

5f.
+ Create a subscription in <literal>node2</literal> to subscribe to
+ <literal>node3</literal>:
-> "on"

5g.
+ Create a subscription in <literal>node3</literal> to subscribe to
+ <literal>node1</literal>. Use <literal>copy_data = force </literal> so that
+ the existing table data is copied during initial sync:
-> "on"

5h.
+ Create a subscription in <literal>node3</literal> to subscribe to
+ <literal>node2</literal>. Use <literal>copy_data = off</literal>
-> "on"

5i.
+ <literal>node3</literal>. Incremental changes made in any node will be
+ replicated to the other two nodes.
"in any node" -> "on any node"

~~~

6. doc/src/sgml/logical-replication.sgml - Adding a new node when data
is present in the new node

+ <title>Adding a new node when data is present in the new node</title>
SUGGESTION
Adding a new node when table data is present on the new node

~~~

7. doc/src/sgml/logical-replication.sgml - Generic steps for adding a
new node to an existing set of nodes

7a.
+ <para>
+ Step-2: Lock the required tables of the new node in EXCLUSIVE mode until
+ the setup is complete. (This lock is necessary to prevent any modifications
+ from happening in the new node because if data modifications occurred after
+ Step-3, there is a chance that the modifications will be published to the
+ first node and then synchronized back to the new node while creating the
+ subscription in Step-5. This would result in inconsistent data).
+ </para>
"happening in the new node" -> "happening on the new node"

7b.
+ not be synchronized to the new node. This would result in inconsistent
+ data. There is no need to lock the required tables in
+ <literal>node1</literal> because any data changes made will be synchronized
+ while creating the subscription with <literal>copy_data = force</literal>).
+ </para>
"no need to lock the required tables in" -> "no need to lock the
required tables on"

======

8. doc/src/sgml/ref/create_subscription.sgml

@@ -403,7 +403,10 @@ CREATE SUBSCRIPTION <replaceable
class="parameter">subscription_name</replaceabl
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>.
+ <literal>copy_data = force</literal>. Refer to
+ <xref linkend="logical-replication-bidirectional"/> for how
+ <literal>copy_data</literal> and <literal>origin</literal> can be used
+ in bidirectional replication.
</para>

"can be used in bidirectional replication" -> "can be used to set up
bidirectional replication"

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