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