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
