Re: Documentation to upgrade logical replication cluster

On Mon, 15 Jan 2024 at 09:01, Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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>

Modified

> ======
> 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"

Added

> ~
>
> 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:"
> ~~~

Modified

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

Modified

> ~~~
>
> 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?

Modified

>
> //////////
> 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.

Modified

> ~
>
> 5b
> /which is/which are/

Modified

> ~~~
>
> 6.
> +       <step>
> +        <para>
> +         Initialize data1_upgraded instance by using the required newer
> +         version.
> +        </para>
> +       </step>
>
> data1_upgraded should be rendered as literal.

Modified

> ~~~
>
> 7.
> +
> +       <step>
> +        <para>
> +         Initialize data2_upgraded instance by using the required newer
> +         version.
> +        </para>
> +       </step>
>
> data2_upgraded should be rendered as literal.

Modified

> ~~~
>
> 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?

Modified

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

This is not required after using xref, removed it.

> //////////
> 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"

Changed it to keep it consistent

> ~~~
>
> 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.

Modified

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

Modified

> ~~~
>
> 11.
> +
> +       <step>
> +        <para>
> +         Initialize data1_upgraded instance by using the required
> newer version.
> +        </para>
> +       </step>
>
> data1_upgraded should be rendered as literal.

Modified

> ~~~
>
> 12.
> +
> +       <step>
> +        <para>
> +         Initialize data2_upgraded instance by using the required
> newer version.
> +        </para>
> +       </step>
>
> data2_upgraded should be rendered as literal.

Modified

> ~~~
>
> 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?

Modified

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

This is not required after using xref, removed it.

> ~~~
>
> 14.
> +
> +       <step>
> +        <para>
> +         Initialize data3_upgraded instance by using the required
> newer version.
> +       </para>
> +       </step>
>
> data3_upgraded should be rendered as literal.

Modified

> ~~~
>
> 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.:

Changed it to xref.

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

This is not required after using xref, removed it.

> //////////
> 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.

Modified

> ~
>
> 16b
> /which is/which are/

Modified

> ~~~
>
> 17.
> +
> +       <step>
> +        <para>
> +         Initialize data1_upgraded instance by using the required newer
> +         version.
> +        </para>
> +       </step>
>
> data1_upgraded should render as literal.

Modified

> ~~~
>
> 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?

Modified to xref

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

This is not required after using xref, removed it.

> ~
>
> 18c.
> /Create any/create any/

Modified

> ~~~
>
> 19.
> +
> +       <step>
> +        <para>
> +         Initialize data2_upgraded instance by using the required newer
> +         version.
> +        </para>
> +       </step>
>
> data2_upgraded should render as literal.

Modified

> ~~~
>
> 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.:

Modified to xref

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

This is not required after using xref, removed it.

Thanks for the comments, the attached v3 version patch has the changes
for the same.

Regards,
Vignesh