Re: Handle infinite recursion in logical replication setup

On Fri, Jun 10, 2022 at 2:09 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Below are some review comments for the patch v18-0004
>
> ======
>
> 1. Commit message
>
> Document the steps for the following:
> a) Create a two-node bidirectional replication when there is no data in both the
> 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.
> d) Generic steps to add a new node to the existing set of nodes.
>
> SUGGESTION (minor changes to make the tense consistent)
> Documents the steps for the following:
> 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.
> d) Generic steps for adding a new node to an existing set of nodes.

Modified

> ======
>
> 2. doc/src/sgml/logical-replication.sgml - blurb
>
> +   <para>
> +    Bidirectional replication is useful in creating a multi-master database
> +    which helps in performing read/write operations from any of the nodes.
>
> SUGGESTION
> Bidirectional replication is useful for creating a multi-master
> database environment for replicating read/write operations performed
> by any of the member nodes.

Modified

> ~~~
>
> 3. doc/src/sgml/logical-replication.sgml - warning
>
> +   <warning>
> +    <para>
> +     Setting up bidirectional logical replication across nodes
> requires multiple
> +     steps to be performed on various nodes. Because not all operations are
> +     transactional, the user is advised to take backups.
> +    </para>
> +   </warning>
>
> SUGGESTION (but keep your formatting)
> Setting up bidirectional logical replication requires multiple steps
> to be performed on various nodes. Because...

Modified

> ~~~
>
> 4. doc/src/sgml/logical-replication.sgml - Setting bidirectional
> replication between two nodes
>
> +   <para>
> +    The steps to create a two-node bidirectional replication when there is no
> +    data in both the nodes <literal>node1</literal> and
> +    <literal>node2</literal> are given below:
> +   </para>
>
> SUGGESTION (but keep your formatting)
> The following steps demonstrate how to create a two-node bidirectional
> replication when there is no table data present in both nodes node1
> and node2:

Modified

> ~~~
>
> 5. doc/src/sgml/logical-replication.sgml - Setting bidirectional
> replication between two nodes
>
> +   <para>
> +    Lock the required tables of <literal>node1</literal> and
> +    <literal>node2</literal> in <literal>EXCLUSIVE</literal> mode until the
> +    setup is completed.
> +   </para>
>
> Instead of "the required tables" shouldn't this just say "table t1"?

Modified

> ~~~
>
> 6. doc/src/sgml/logical-replication.sgml - Setting bidirectional
> replication between two nodes
>
> +   <para>
> +    Now the bidirectional logical replication setup is complete between
> +    <literal>node1</literal>, <literal>node2</literal> and
> +    <literal>node2</literal>. Any subsequent changes in one node will
> +    replicate the changes to the other nodes.
> +   </para>
>
> SUGGESTION (for 2nd sentence, and keep your formatting)
> Any incremental changes from node1 will be replicated to node2, and
> any incremental changes from node2 will be replicated to node1.

Modified

> ~~~
>
> 7. doc/src/sgml/logical-replication.sgml - Adding a new node when
> there is no data in any of the nodes
>
> +   <para>
> +    Adding a new node <literal>node3</literal> to the existing
> +    <literal>node1</literal> and <literal>node2</literal> when there is no data
> +    in any of the nodes requires setting up subscription in
> +    <literal>node1</literal> and <literal>node2</literal> to replicate the data
> +    from <literal>node3</literal> and setting up subscription in
> +    <literal>node3</literal> to replicate data from <literal>node1</literal>
> +    and <literal>node2</literal>.
> +   </para>
>
> SUGGESTION (but keep your formatting)
> The following steps demonstrate adding a new node node3 to the
> existing node1 and node2 when there is no t1 data in any of the nodes.
> This requires creating subscriptions in node1 and node2 to replicate
> the data from node3 and creating subscriptions in node3 to replicate
> data from node1 and node2.

Modified

> ~~~
>
> 8. doc/src/sgml/logical-replication.sgml - Adding a new node when
> there is no data in any of the nodes
>
> +   <note>
> +    <para>
> +     It is assumed that the bidirectional logical replication between
> +     <literal>node1</literal> and <literal>node2</literal> is already
> completed.
> +    </para>
> +   </note>
>
> IMO this note should just be a text note in the previous paragraph
> instead of an SGML <note>. e.g. "Note: These steps assume that..."

Modified

> ~~~
>
> 9. doc/src/sgml/logical-replication.sgml - Adding a new node when
> there is no data in any of the nodes
>
> +   <para>
> +    Lock the required tables of all the nodes <literal>node1</literal>,
> +    <literal>node2</literal> and <literal>node3</literal> in
> +    <literal>EXCLUSIVE</literal> mode until the setup is completed.
> +   </para>
>
> Instead of "the required tables" shouldn't this just say "table t1"?

Modified

> ~~~
>
> 10. doc/src/sgml/logical-replication.sgml - Adding a new node when
> there is no data in any of the nodes
>
> +   <para>
> +    Now the bidirectional logical replication setup is complete between
> +    <literal>node1</literal>, <literal>node2</literal> and
> +    <literal>node2</literal>. Any subsequent changes in one node will
> +    replicate the changes to the other nodes.
> +   </para>
>
> SUGGESTION (2nd sentence)
> Incremental changes made in any node will be replicated to the other two nodes.

Modified

> ~~~
>
> 11. doc/src/sgml/logical-replication.sgml - Adding a new node when
> data is present in the existing nodes
>
> +    <para>
> +     Adding a new node <literal>node3</literal> which has no data to the
> +     existing <literal>node1</literal> and <literal>node2</literal> when data
> +     is present in existing nodes <literal>node1</literal> and
> +     <literal>node2</literal> needs similar steps. The only change required
> +     here is that <literal>node3</literal> should create a subscription with
> +     <literal>copy_data = force</literal> to one of the existing nodes to
> +     receive the existing data during initial data synchronization.
> +   </para>
>
> SUGGESTION (but keep your formatting)
> The following steps demonstrate adding a new node node3 which has no
> t1 data to the existing node1 and node2 where t1 data is present. This
> needs similar steps; the only change required here is that node3
> should create a subscription with copy_data = force to one of the
> existing nodes so it can receive the existing t1 data during initial
> data synchronization.

Modified

> ~~~
>
> 12 doc/src/sgml/logical-replication.sgml - Adding a new node when data
> is present in the existing nodes
>
> +   <note>
> +    <para>
> +     It is assumed that the bidirectional logical replication between
> +     <literal>node1</literal> and <literal>node2</literal> is already
> completed.
> +     The nodes <literal>node1</literal> and <literal>node2</literal> has some
> +     pre-existing data in table t1 that is synchronized in both the nodes.
> +    </para>
> +   </note>
>
> 12a.
> IMO this note should just be text in the previous paragraph instead of
> an SGML <note>. e.g. "Note: These steps assume that..."

Modified

> 12b.
> SUGGESTION (minor rewording; keep your formatting)
> Note: These steps assume that the bidirectional logical replication
> between node1 and node2 is already completed, and the pre-existing
> data in table t1 is already synchronized in both those nodes.

Modified

> ~~~
>
> 13. doc/src/sgml/logical-replication.sgml - Adding a new node when
> data is present in the existing nodes
>
> +   <para>
> +    Lock the required tables of <literal>node2</literal> and
> +    <literal>node3</literal> in <literal>EXCLUSIVE</literal> mode until the
> +    setup is completed. No need to lock the tables in <literal>node1</literal>
> +    as any data changes made will be synchronized while creating the
> +    subscription with <literal>copy_data</literal> specified as
> +    <literal>force</literal>.
> +   </para>
>
> 13a.
> Instead of "the required tables" shouldn't this just say "table t1"?

Modified

> 13b.
> SUGGESTION (2nd sentence; keep your formatting)
> There is no need to lock table t1 in node1 because any data changes
> made will be synchronized while creating the subscription with
> copy_data = force.

Modified

> ~~~
>
> 14. doc/src/sgml/logical-replication.sgml - Adding a new node when
> data is present in the existing nodes
>
> +   <para>
> +    Create a subscription in <literal>node3</literal> to subscribe to
> +    <literal>node1</literal>. Use <literal>copy_data</literal> specified as
> +    <literal>force</literal> so that the existing table data is
> +    copied during initial sync:
>
> SUGGESTION (2nd sentence; keep your formatting)
> Use copy_data = force so that the existing table data is copied during
> the initial sync:

Modified

> ~~~
>
> 15. doc/src/sgml/logical-replication.sgml - Adding a new node when
> data is present in the existing nodes
>
> +   <para>
> +    Create a subscription in <literal>node3</literal> to subscribe to
> +    <literal>node2</literal>. Use <literal>copy_data</literal> specified as
> +    <literal>off</literal> because the initial table data would have been
> +    already copied in the previous step:
>
> SUGGESTION (2nd sentence; keep your formatting)
> Use copy_data = off because the initial table data would have been
> already copied in the previous step:

Modified

> ~~~
>
> 16. doc/src/sgml/logical-replication.sgml - Adding a new node when
> data is present in the existing nodes
>
> +   <para>
> +    Now the bidirectional logical replication setup is complete between
> +    <literal>node1</literal>, <literal>node2</literal> and
> +    <literal>node2</literal>. Any subsequent changes in one node will
> +    replicate the changes to the other nodes.
> +   </para>
>
> 16a.
> Should say node1, node2 and node3

Modified it in v19

> 16b.
> SUGGESTION (2nd sentence – same as the previous comment)
> Incremental changes made in any node will be replicated to the other two nodes.

Modified

> ~~~
>
> 17. doc/src/sgml/logical-replication.sgml - Adding a new node when
> data is present in the new node
>
> +   <warning>
> +    <para>
> +     Adding a new node <literal>node3</literal> to the existing
> +     <literal>node1</literal> and <literal>node2</literal> when data is present
> +     in the new node <literal>node3</literal> is not possible.
> +    </para>
> +   </warning>
>
> 17a.
> IMO
> - Not really necessary to name the nodes, because this is a generic statement
> - Maybe say "not supported" instead of "not possible". e.g. there is
> no ERROR check for this case is there?
>
> SUGGESTION
> Adding a new node when data is present in the new node tables is not supported.

Modified

> 17b.
> I am not sure but I felt this advice seemed more like an SGML <note>;
> note a <warning>

Modified

> ~~~
>
> 18. doc/src/sgml/logical-replication.sgml - Generic steps to add a new
> node to the existing set of nodes
>
> +   <title>Generic steps to add a new node to the existing set of nodes</title>
>
> SUGGESTION
> Generic steps for adding a new node to an existing set of nodes

Modified

> ~~~
>
> 19. doc/src/sgml/logical-replication.sgml - Generic steps to add a new
> node to the existing set of nodes
>
> +   <para>
> +    1. Create the required publication on the new node.
> +   </para>
> +   <para>
> +    2. Lock the required tables of the new node in <literal>EXCLUSIVE</literal>
> +    mode until the setup is complete. This is required to prevent any
> +    modifications from happening in the new node. If data modifications occur
> +    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
> +    subscription in step-5 resulting in inconsistent data.
> +   </para>
> +   <para>
> +    3. Create subscriptions on existing nodes pointing to publication on
> +    the new node with <literal>origin</literal> parameter specified as
> +    <literal>local</literal> and <literal>copy_data</literal> specified as
> +    <literal>off</literal>.
> +   </para>
> +   <para>
> +    4. Lock the required tables of the existing nodes except the first node in
> +    <literal>EXCLUSIVE</literal> mode until the setup is complete. This is
> +    required to prevent any modifications from happening. If data modifications
> +    occur, there is a chance that the modifications done in between step-5 and
> +    step-6 will not be synchronized to the new node resulting in inconsistent
> +    data. No need to lock the tables in the first node as any data changes
> +    made will be synchronized while creating the subscription with
> +    <literal>copy_data</literal> specified as <literal>force</literal>.
> +   </para>
> +   <para>
> +    5. Create a subscription on the new node pointing to publication on the
> +    first node with <literal>origin</literal> parameter specified as
> +    <literal>local</literal> and <literal>copy_data</literal> parameter
> +    specified as <literal>force</literal>.
> +   </para>
> +   <para>
> +    6. Create subscriptions on the new node pointing to publications on the
> +    remaining nodes with <literal>origin</literal> parameter specified as
> +    <literal>local</literal> and <literal>copy_data</literal> parameter
> +    specifiedas <literal>off</literal>.
> +   </para>
>
> Following suggestions make the following changes:
> - Some minor rewording
> - Change the step names
> - Use "=" instead of "specified as" for the parameters
> - I felt it is more readable if the explanatory notes are separate
> paragraphs from the steps. Maybe if you could indent them or something
> it would be even better.
> - Added a couple more explanatory notes
>
> SUGGESTION (using those above changes; please keep your formatting)
>
> Step-1: Create a publication on the new node.
>
> 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).
>
> Step-3. Create subscriptions on existing nodes to the publication on
> the new node with origin = local and copy_data = off.
>
> (The copy_data = off is OK here because it is asserted that the
> published tables of the new node will have no pre-existing data).
>
> Step-4. Lock the required tables of the existing nodes except the
> first node in EXCLUSIVE mode until the setup is complete.
>
> (This lock is necessary to prevent any modifications from happening.
> If data modifications occur, there is a chance that modifications done
> between Step-5 and Step-6 will not be synchronized to the new node.
> This would result in inconsistent data. There is no need to lock table
> t1 in node1 because any data changes made will be synchronized while
> creating the subscription with copy_data = force).
>
> Step-5. Create a subscription on the new node to the publication on
> the first node with origin = local and copy_data = force.
>
> (This will copy the same table data from the existing nodes to the new node)
>
> Step-6. Create subscriptions on the new node to publications on the
> remaining nodes with origin = local and copy_data  = off.
>
> (The copy_data = off is OK here because the existing node data was
> already copied to the new node in Step-5)

Modified

> ======
>
> 20. doc/src/sgml/ref/create_subscription.sgml
>
> -   <literal>copy_data = force</literal>.
> +   <literal>copy_data = force</literal>. Refer to the
> +   <xref linkend="logical-replication-bidirectional"/> on how
> +   <literal>copy_data</literal> and <literal>origin</literal> can be used
> +   in bidirectional replication.
>    </para>
>
> SUGGESTION (keep your formatting)
> Refer to <xref linkend="logical-replication-bidirectional"/> for how
> <literal>copy_data</literal> and <literal>origin</literal> can be used
> in bidirectional replication.

Modified

Thanks for the comments. The comments are handled as part of the v20
patch attached at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm0XtQVX3taeLKWE-gPQyppqs34ipXawAPOyO%3Dhe37MQSg%40mail.gmail.com

Regards,
Vignesh