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