On Fri, Apr 29, 2022 at 8:08 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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".
Modified
> ~~~
>
> 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.
Modified
> ~~~
>
> 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.
Modified
The attached v12 patch has the changes for the same.
Regards,
Vignesh