Re: Handle infinite recursion in logical replication setup

On Fri, May 27, 2022 at 10:56 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here are some more review comments for v15-0002 I found while working
> through the documented examples. These are all for
> doc/src/sgml/logical-replication.sgml. The section numbers (e.g.
> 31.11.1) are the assigned numbers after HTML rendering.
>
> The general comment is that the sections appeared at first to be
> independent of each other, but they are not really - sometimes
> existing data and existing pub/sub are assumed to remain. Therefore, I
> think some more text needs to be added to clarify the initial state
> for each of these sections.
>
> ======
>
> 1. Section id.
>
> <sect1 id="bidirectional-logical-replication">
>
> IMO this section ID should be renamed
> "logical-replication-bidirectional" then it will be more consistent
> filenames with all the other logical replication section names.

Modified

> ~~~
>
> 2. Section 31.11.2. Adding a new node when there is no data in any of the nodes
>
> 2a.
> This seems like a continuation of 31.11.1 because pub/sub of
> node1,node2 is assumed to exist still.

Added a note

> 2b.
> The bottom of this section should say after these steps any subsequent
> incremental data changes on any node will be replicated to all nodes
> (node1, node2, node3)

Modified

> ~~~
>
> 3. Section 31.11.3. Adding a new node when data is present in the existing nodes
>
> 3a.
> This seems to be a continuation of 31.11.1 because pub/sub (and
> initial data) of node1/node2 is assumed to exist.

Added a note

> 3b.
>
> The bottom of this section should say after these steps that any
> initial data of node1/node2 will be seen in node3, and any subsequent
> incremental data changes on any node will be replicated to all nodes
> (node1, node2, node3)

Modified

> ~~~
>
> 4. Section 31.11.4. Adding a new node when data is present in the new node
>
> 4a.
> This seems to be a continuation of 31.11.1 because pub/sub (and
> initial data) of node1/node2 is assumed to exist.

This section contents have been removed because this is not feasible currently

> 4b.
> It is not made very clear up-front if the tables on node1 and node2
> are empty or not. Apparently, they are considered NOT empty because
> later in the example you are using "force" when you create the
> subscription on node3.

This section contents have been removed because this is not feasible currently

> 4c.
> The SQL wrapping here is strangely different from others (put the
> subscription name on 1st line)
> node3=# CREATE SUBSCRIPTION
> node3-# sub_node3_node1 CONNECTION 'dbname=foo host=node1 user=repuser'
> node3-# PUBLICATION pub_node1
> node3-# WITH (copy_data = force, only_local = on);
> CREATE SUBSCRIPTION

This section contents have been removed because this is not feasible currently

> 4d.
> The SQL wrapping here is strangely different from others (put the
> subscription name on 1st line)
> node3=# CREATE SUBSCRIPTION
> node3-# sub_node3_node2 CONNECTION 'dbname=foo host=node2 user=repuser'
> node3-# PUBLICATION pub_node2
> node3-# WITH (copy_data = off, only_local = on);
> CREATE SUBSCRIPTION

This section contents have been removed because this is not feasible currently

> 4e.
> The bottom of this section should say after this that any initial data
> of node1/node2 will be seen in node3, and any subsequent incremental
> data changes on any node will be replicated to all nodes (node1,
> node2, node3)

This section contents have been removed because this is not feasible currently

> ~~~
>
> 5. Section 31.11.5. Generic steps to add a new node to the existing set of nodes
>
> 5a
> Create subscriptions on the new node pointing to publication on the
> first node with only_local option specified as on and copy_data option
> specified as "force".
>
> -> that should say "Create a subscription" (singular)

Modified

> 5b.
> Create subscriptions on the new node pointing to publications on the
> remaining node with only_local option specified as on and copy_data
> option specified as off.
>
> -> that should say "on the remaining node" (plural)

Modified

Thanks for the comments, the v17 patch attached at [1] has the changes
for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm1rMihO7daiFyLdxkqArrC%2BdtM61oPXc-XrTYBYnJg3nw%40mail.gmail.com

Regards,
Vignesh