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

Here are my review comments for the v11* patches.

======

v11-0001 - no more comments. LGTM

======

V11-0002

1. doc/src/sgml/logical-replication.sgml

+   <para>
+     Bidirectional replication is useful in creating multi master database
+     which helps in performing read/write operations from any of the nodes.
+     Setting up bidirectional logical replication between two nodes requires
+     creation of the publication in all the nodes, creating subscriptions in
+     each of the nodes that subscribes to data from all the nodes. The steps
+     to create a two-node bidirectional replication are given below:
+   </para>

Wording: "creating multi master database" -> "creating a multi-master database"
Wording: "creation of the publication in all the nodes" -> "creation
of a publication in all the nodes".

~~~

2. doc/src/sgml/logical-replication.sgml

> +<programlisting>
> +node2=# CREATE SUBSCRIPTION sub_node1_node2
> +node2=# CONNECTION 'dbname=foo host=node1 user=repuser'
> +node2=# PUBLICATION pub_node1
> +node2=# WITH (copy_data = off, local_only = on);
> +CREATE SUBSCRIPTION
> +</programlisting>
> + </para>
>
> IIUC the closing </para> should be on the same line as the
> </programlisting>. I recall there was some recent github push about
> this sometime in the last month - maybe you can check to confirm it.

Vignesh: I have seen the programlisting across multiple places and noticed
that there is no such guideline being followed. I have not made any
change for this comment.

FYI – I found the push [1] by PeterE that I was referring to so. I
thought we perhaps should follow this, even if not all other code is
doing so. But you can choose.

~~~

3. doc/src/sgml/logical-replication.sgml

+   <para>
+    Create a subscription in <literal>node3</literal> to subscribe the changes
+    from <literal>node1</literal> and <literal>node2</literal>, here
+    <literal>copy_data</literal> is specified as <literal>force</literal> when
+    creating a subscription to <literal>node1</literal> so that the existing
+    table data is copied during initial sync:
+<programlisting>
+node3=# CREATE SUBSCRIPTION
+node3-# sub_node3_node1 CONNECTION 'dbname=foo host=node1 user=repuser'
+node3-# PUBLICATION pub_node1
+node3-# WITH (copy_data = force, local_only = on);
+CREATE SUBSCRIPTION
+node3=# CREATE SUBSCRIPTION
+node3-# sub_node3_node2 CONNECTION 'dbname=foo host=node2 user=repuser'
+node3-# PUBLICATION pub_node2
+node3-# WITH (copy_data = off, local_only = on);
+CREATE SUBSCRIPTION
+</programlisting>

I think this part should be split into 2 separate program listings
each for the different subscriptions. Then those descriptions can be
described separately (e.g. why one is force and the other is not).
Then it will also be more consistent with how you split/explained
something similar in the previous section on this page.

------
[1] https://github.com/postgres/postgres/commit/d7ab2a9a3c0a2800ab36bb48d1cc97370067777e

Kind Regards,
Peter Smith.
Fujitsu Australia