Re: Documentation to upgrade logical replication cluster

Hi Vignesh, here are some review comments for patch v2-0001.

======
doc/src/sgml/ref/pgupgrade.sgml

1.
+ <step id="pgupgrade-step-logical-replication">
+ <title>Upgrade logical replication clusters</title>
+
+ <para>
+ Refer <link linkend="logical-replication-upgrade">logical
replication upgrade section</link>
+ for details on upgrading logical replication clusters.
+ </para>
+
+ </step>
+

This renders like:
Refer logical replication upgrade section for details on upgrading
logical replication clusters.

~

IMO it would be better to use xref instead of link, which will render
more normally like:
See Section 30.11 for details on upgrading logical replication clusters.

SUGGESTION
<para>
See <xref linkend="logical-replication-upgrade"/>
for details on upgrading logical replication clusters.
</para>

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

2. GENERAL - blurb

+ <sect1 id="logical-replication-upgrade">
+ <title>Upgrade</title>
+
+ <procedure>
+ <step id="prepare-publisher-upgrades">
+ <title>Prepare for publisher upgrades</title>

I felt there should be a short (1 or 2 sentence) general blurb about
pub/sub upgrade before jumping straight into:

"1. Prepare for publisher upgrades"
"2. Prepare for subscriber upgrades"
"3. Upgrading logical replication cluster"

~

Specifically, at first, it looks strange that the HTML renders as
steps 1,2,3 instead of sub-sections (30.11.1, 30.11.2, 30.11.3); Maybe
"steps" are fine, but then at least there needs to be some intro
sentence saying like "follow these steps:"

~~~

3.
+ <step id="upgrading-logical-replication-cluster">
+ <title>Upgrading logical replication cluster</title>

/cluster/clusters/

~~~

4.
+ <para>
+ The steps to upgrade the following logical replication clusters are
+ detailed below:
+ <itemizedlist>
+ <listitem>
+ <para>
+ <link linkend="steps-two-node-logical-replication-cluster">Two-node
logical replication cluster.</link>
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ <link linkend="steps-cascaded-logical-replication-cluster">Cascaded
logical replication cluster.</link>
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ <link linkend="steps-two-node-circular-logical-replication-cluster">Two-node
circular logical replication cluster.</link>
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para>

Isn't there a better way to accomplish this by using xref and
'xreflabel' so you don't have to type the link text here?

//////////
Steps to upgrade a two-node logical replication cluster
//////////

5.
+ <para>
+ Let's say publisher is in <literal>node1</literal> and subscriber is
+ in <literal>node2</literal>. The subscriber <literal>node2</literal> has
+ two subscriptions sub1_node1_node2 and sub2_node1_node2 which is
+ subscribing the changes from <literal>node1</literal>.
+ </para>

5a
Those subscription names should also be rendered as literals.

~

5b
/which is/which are/

~~~

6.
+ <step>
+ <para>
+ Initialize data1_upgraded instance by using the required newer
+ version.
+ </para>
+ </step>

data1_upgraded should be rendered as literal.

~~~

7.
+
+ <step>
+ <para>
+ Initialize data2_upgraded instance by using the required newer
+ version.
+ </para>
+ </step>

data2_upgraded should be rendered as literal.

~~~

8.
+
+ <step>
+ <para>
+ On <literal>node2</literal>, create any tables that were created in
+ the upgraded publisher <literal>node1</literal> server between
+ <link linkend="two-node-cluster-disable-subscriptions-node2">
+ when the subscriptions where disabled in
<literal>node2</literal></link>
+ and now, e.g.:
+<programlisting>
+node2=# CREATE TABLE distributors (did integer PRIMARY KEY, name varchar(40));
+CREATE TABLE
+</programlisting>
+ </para>
+ </step>

8a.
This link to the earlier step renders badly like:
On node2, create any tables that were created in the upgraded
publisher node1 server between when the subscriptions where disabled
in node2 and now, e.g.:

IMO this link should be like "Step N", not some words -- maybe it is
another opportunity for using xreflabel?

~

8b.
Also has typos "when the subscriptions where disabled" (??)

//////////
Steps to upgrade a cascaded logical replication clusters
//////////

9.
+ <procedure>
+ <step id="steps-cascaded-logical-replication-cluster">
+ <title>Steps to upgrade a cascaded logical replication clusters</title>

The title has a strange mix of singular "a" and plural "clusters"

~~~

10.
+ <para>
+ Let's say we have a cascaded logical replication setup
+ <literal>node1</literal>-><literal>node2</literal>-><literal>node3</literal>.
+ Here <literal>node2</literal> is subscribing the changes from
+ <literal>node1</literal> and <literal>node3</literal> is subscribing
+ the changes from <literal>node2</literal>. The <literal>node2</literal>
+ has two subscriptions sub1_node1_node2 and sub2_node1_node2 which is
+ subscribing the changes from <literal>node1</literal>. The
+ <literal>node3</literal> has two subscriptions sub1_node2_node3 and
+ sub2_node2_node3 which is subscribing the changes from
+ <literal>node2</literal>.
+ </para>

10a.
Those subscription names should also be rendered as literals.

~

10b.
/which is/which are/ (occurs 2x)

~~~

11.
+
+ <step>
+ <para>
+ Initialize data1_upgraded instance by using the required
newer version.
+ </para>
+ </step>

data1_upgraded should be rendered as literal.

~~~

12.
+
+ <step>
+ <para>
+ Initialize data2_upgraded instance by using the required
newer version.
+ </para>
+ </step>

data2_upgraded should be rendered as literal.

~~~

13.
+
+ <step>
+ <para>
+ On <literal>node2</literal>, create any tables that were created in
+ the upgraded publisher <literal>node1</literal> server between
+ <link linkend="cascaded-cluster-disable-sub-node1-node2">
+ when the subscriptions where disabled in
<literal>node2</literal></link>
+ and now, e.g.:
+<programlisting>
+node2=# CREATE TABLE distributors (did integer PRIMARY KEY, name varchar(40));
+CREATE TABLE
+</programlisting>
+ </para>
+ </step>

13a.
This link to the earlier step renders badly like:
On node2, create any tables that were created in the upgraded
publisher node1 server between when the subscriptions where disabled
in node2 and now, e.g.:

IMO this link should be like "Step N", not some words -- maybe it is
another opportunity for using xreflabel?

~

13b
Also has typos "when the subscriptions where disabled" (??)

~~~

14.
+
+ <step>
+ <para>
+ Initialize data3_upgraded instance by using the required
newer version.
+ </para>
+ </step>

data3_upgraded should be rendered as literal.

~~~

15.
+
+ <step>
+ <para>
+ On <literal>node3</literal>, create any tables that were created in
+ the upgraded <literal>node2</literal> between
+ <link linkend="cascaded-cluster-disable-sub-node2-node3">when the
+ subscriptions where disabled in <literal>node3</literal></link>
+ and now, e.g.:
+<programlisting>
+node3=# CREATE TABLE distributors (did integer PRIMARY KEY, name varchar(40));
+CREATE TABLE
+</programlisting>
+ </para>
+ </step>

15a.
This link to the earlier step renders badly like:
On node3, create any tables that were created in the upgraded node2
between when the subscriptions where disabled in node3 and now, e.g.:

~

15b.
Also has typos "when the subscriptions where disabled" (??)

//////////
Steps to upgrade a two-node circular logical replication cluster
//////////

16.
+ <para>
+ Let's say we have a circular logical replication setup
+ <literal>node1</literal>-><literal>node2</literal> and
+ <literal>node2</literal>-><literal>node1</literal>. Here
+ <literal>node2</literal> is subscribing the changes from
+ <literal>node1</literal> and <literal>node1</literal> is subscribing
+ the changes from <literal>node2</literal>. The <literal>node1</literal>
+ has two subscriptions sub1_node2_node1 and sub2_node2_node1 which is
+ subscribing the changes from <literal>node2</literal>. The
+ <literal>node2</literal> has two subscriptions sub1_node1_node2 and
+ sub2_node1_node2 which is subscribing the changes from
+ <literal>node1</literal>.
+ </para>

16a
Those subscription names should also be rendered as literals.

~

16b
/which is/which are/

~~~

17.
+
+ <step>
+ <para>
+ Initialize data1_upgraded instance by using the required newer
+ version.
+ </para>
+ </step>

data1_upgraded should render as literal.

~~~

18.
+
+ <step>
+ <para>
+ On <literal>node1</literal>, Create any tables that were created in
+ <literal>node2</literal> between <link
linkend="circular-cluster-disable-sub-node2">
+ when the subscriptions where disabled in
<literal>node2</literal></link>
+ and now, e.g.:
+<programlisting>
+node1=# CREATE TABLE distributors (did integer PRIMARY KEY, name varchar(40));
+CREATE TABLE
+</programlisting>
+ </para>
+ </step>

18a.
This link to the earlier step renders badly like:
On node1, Create any tables that were created in node2 between when
the subscriptions where disabled in node2 and now, e.g.:

IMO this link should be like "Step N", not some words -- maybe it is
another opportunity for using xreflabel?
~

18b
Also has typos "when the subscriptions where disabled" (??)

~

18c.
/Create any/create any/

~~~

19.
+
+ <step>
+ <para>
+ Initialize data2_upgraded instance by using the required newer
+ version.
+ </para>
+ </step>

data2_upgraded should render as literal.

~~~

20.
+
+ <step>
+ <para>
+ On <literal>node2</literal>, Create any tables that were created in
+ the upgraded <literal>node1</literal> between <link
linkend="circular-cluster-disable-sub-node1">
+ when the subscriptions where disabled in
<literal>node1</literal></link>
+ and now, e.g.:
+<programlisting>
+node2=# CREATE TABLE distributors (did integer PRIMARY KEY, name varchar(40));
+CREATE TABLE
+</programlisting>
+ </para>
+ </step>

20a.
This link to the earlier step renders badly like:
On node2, Create any tables that were created in the upgraded node1
between when the subscriptions where disabled in node1 and now, e.g.:

~

20b
Also has typos "when the subscriptions where disabled" (??)

~

20c.
/Create any/create any/

======
Kind Regards,
Peter Smith.
Fujitsu Australia