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

On Wed, Jun 15, 2022 at 12:13 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

> ~~~
>
> 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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

> ~~~
>
> 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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

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

Modified

> ~~~
>
> 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

Modified

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

Modified

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

Modified

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

Modified

Thanks for the comments, the v21 patch attached at [1] has the changes
for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm3%2B6cey0rcDft1ZUCjSUtLDM0xmU_Q%2BYhcsBrqe1RH8%3Dw%40mail.gmail.com

Regards,
Vignesh