Re: Documentation to upgrade logical replication cluster

On Fri, 5 Jan 2024 at 10:49, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here are some review comments for patch v1-0001.
>
> ======
> doc/src/sgml/ref/pgupgrade.sgml
>
> 1. GENERAL - blank lines
>
> Most (but not all) of your procedure steps are preceded by blank lines
> to make them more readable in the SGML. Add the missing blank lines
> for the steps that didn't have them.

Modified

> 2. GENERAL - for e.g.:
>
> All the "for e.g:" that precedes the code examples can just say
> "e.g.:" like in other examples on this page.

Modified

> ~~~
> 3. GENERAL - reference from elsewhere
>
> I was wondering if "Chapter 30. Logical Replication" should include a
> section that references back to all this just to make it easier to
> find.

I have moved this to Chapter 30 now as it is more applicable there and
also based on feedback from Amit at [1].

> ~~~
>
> 4.
> + <para>
> + Migration of logical replication clusters can be done when all the members
> + of the old logical replication clusters are version 17.0 or later.
> + </para>
>
> /can be done when/is possible only when/

Modified

> ~~~
>
> 5.
> + <para>
> + The prerequisites of publisher upgrade applies to logical Replication
> + cluster upgrades also. See <xref linkend="prepare-publisher-upgrades"/>
> + for the details of publisher upgrade prerequisites.
> + </para>
>
> /applies to/apply to/
> /logical Replication/logical replication/

Modified

> ~~~
>
> 6.
> + <para>
> + The prerequisites of subscriber upgrade applies to logical Replication
> + cluster upgrades also. See <xref linkend="prepare-subscriber-upgrades"/>
> + for the details of subscriber upgrade prerequisites.
> + </para>
> + </note>
>
> /applies to/apply to/
> /logical Replication/logical replication/

Modified

> ~~~
>
> 7.
> + <para>
> + The steps to upgrade logical replication clusters in various scenarios are
> + given below.
> + </para>
>
> The 3 titles do not render very prominently, so it is too easy to get
> lost scrolling up and down looking for the different scenarios. If the
> title rendering can't be improved, at least a list of 3 links here
> (like a TOC) would be helpful.

I added a list of these 3 links in the beginning.

> ~~~
>
> //////////
> Steps to Upgrade 2 node logical replication cluster
> //////////
>
> 8. GENERAL - server names
>
> I noticed in this set of steps you called the servers 'pub_data' and
> 'pub_upgraded_data' and 'sub_data' and 'sub_upgraded_data'. I see it
> is easy to read like this, it is also different from all the
> subsequent procedures where the names are just like 'data1', 'data2',
> 'data3', and 'data1_upgraded', 'data2_upgraded', 'data3_upgraded'.
>
> I felt maybe it is better to use a consistent naming for all the procedures.

Modified

> ~~~
>
> 9.
> + <step>
> + <title>Steps to Upgrade 2 node logical replication cluster</title>
>
> SUGGESTION
> Steps to upgrade a two-node logical replication cluster

Modified

> ~~~
>
> 10.
> +
> + <procedure>
> + <step>
> + <para>
> + Let's say publisher is in <literal>node1</literal> and subscriber is
> + in <literal>node2</literal>.
> + </para>
> + </step>
>
> 10a.
> This renders as Step 1. But IMO this should not be a "step" at all --
> it's just a description of the scenario.

Modified

> ~
>
> 10b.
> The subsequent steps refer to subscriptions 'sub1_node1_node2' and
> 'sub2_node1_node2'. IMO it would help with the example code if those
> are named up front here too. e.g.
>
> node2 has two subscriptions for changes from node1:
> sub1_node1_node2
> sub2_node1_node2

Modified

> ~~~
>
> 11.
> + <step>
> + <para>
> + Upgrade the publisher node <literal>node1</literal>'s server to the
> + required newer version, for e.g.:
>
> The wording repeating node/node1 seems complicated.
>
> SUGGESTION
> Upgrade the publisher node's server to the required newer version, e.g.:

Modified

> ~~~
>
> 12.
> + <step>
> + <para>
> + Start the upgraded publisher node
> <literal>node1</literal>'s server, for e.g.:
>
> IMO better to use the similar wording used for the "Stop" step
>
> SUGGESTION
> Start the upgraded publisher server in node1, e.g.:

Modified

> ~~~
>
> 13.
> + <step>
> + <para>
> + Upgrade the subscriber node <literal>node2</literal>'s server to
> + the required new version, for e.g.:
>
> The wording repeating node/node2 seems complicated.
>
> SUGGESTION
> Upgrade the subscriber node's server to the required newer version, e.g.:

Modified

> ~~~
>
> 14.
> + <step>
> + <para>
> + Start the upgraded subscriber node <literal>node2</literal>'s server,
> + for e.g.:
>
> IMO better to use the similar wording used for the "Stop" step
>
> SUGGESTION
> Start the upgraded subscriber server in node2, e.g.:

Modified

> ~~~
>
> 15.
> + <step>
> + <para>
> + Create any tables that were created in the upgraded
> publisher <literal>node1</literal>
> + server between step-5 and now, for e.g.:
> +<programlisting>
> +node2=# CREATE TABLE distributors (
> +node2(# did integer CONSTRAINT no_null NOT NULL,
> +node2(# name varchar(40) NOT NULL
> +node2(# );
> +CREATE TABLE
> +</programlisting>
> + </para>
> + </step>
>
> 15a
> Maybe it is better to have a link to setp5 instead of just hardwiring
> "Step-5" in the text.

Modified

> ~'
>
> 15b.
> I didn't think it was needed to spread the CREATE TABLE across
> multiple lines. It is just a dummy example anyway so IMO better to use
> up less space.

Modified

> ~~~
>
> 16.
> + <step>
> + <para>
> + Refresh the publications using
> + <link
> linkend="sql-altersubscription-params-refresh-publication"><command>ALTER
> SUBSCRIPTION ... REFRESH PUBLICATION</command></link>,
>
> /Refresh the publications/Refresh the subscription's publications/

Modified

> ~~~
>
> //////////
> Steps to upgrade cascaded logical replication clusters
> //////////
>
> (these comments are similar to those in the previous procedure, but I
> will give them all again)
>
> 17.
> + <procedure>
> + <step>
> + <title>Steps to upgrade cascaded logical replication clusters</title>
> + <procedure>
> + <step>
> + <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>.
> + </para>
> + </step>
>
> 17a.
> This renders as Step 1. But IMO this should not be a "step" at all --
> it's just a description of the scenario.

Modified

> ~
>
> 17b.
> The subsequent steps refer to subscriptions 'sub1_node1_node2' and
> 'sub1_node1_node2' and 'sub1_node2_node3' and 'sub2_node2_node3'. IMO
> it would help with the example code if those are named up front here
> too, e.g.
>
> node2 has two subscriptions for changes from node1:
> sub1_node1_node2
> sub2_node1_node2
>
> node3 has two subscriptions for changes from node2:
> sub1_node2_node3
> sub2_node2_node3

Modified

> ~~~
>
> 18.
> + <step>
> + <para>
> + Upgrade the publisher node <literal>node1</literal>'s server to the
> + required newer version, for e.g.:
>
> I'm not sure it is good to call this the publisher node, because in
> this scenario node2 is also a publisher node.
>
> SUGGESTION
> Upgrade the node1 server to the required newer version, e.g.:

Modified

> ~~~
>
> 19.
> + <step>
> + <para>
> + Start the upgraded node <literal>node1</literal>'s server, for e.g.:
>
> SUGGESTION
> Start the upgraded node1's server, e.g.:

Modified

> ~~~
>
> 20.
> + <step>
> + <para>
> + Upgrade the node <literal>node2</literal>'s server to the required
> + new version, for e.g.:
>
> SUGGESTION
> Upgrade the node2 server to the required newer version, e.g.:

Modified

> ~~~
>
> 21.
> + <step>
> + <para>
> + Start the upgraded node <literal>node2</literal>'s server, for e.g.:
>
> SUGGESTION
> Start the upgraded node2's server, e.g.:

Modified

> ~~~
>
> 22.
> + <step>
> + <para>
> + Create any tables that were created in the upgraded
> publisher <literal>node1</literal>
> + server between step-5 and now, for e.g.:
>
> 22a
> Maybe this should say "On node2, create any tables..."

Modified

> ~
>
> 22b.
> Maybe it is better to have a link to step5 instead of just hardwiring
> "Step-5" in the text.

Modified

> ~
>
> 22c.
> I didn't think it was needed to spread the CREATE TABLE across
> multiple lines. It is just a dummy example anyway so IMO better to use
> up less space.

Modified

> ~~~
>
> 23.
> + <step>
> + <para>
> + Enable all the subscriptions on <literal>node2</literal> that are
> + subscribing the changes from <literal>node2</literal> by using
> + <link
> linkend="sql-altersubscription-params-enable"><command>ALTER
> SUBSCRIPTION ... ENABLE</command></link>,
> + for e.g.:
>
> Typo: /subscribing the changes from node2/subscribing the changes from node1/

Modified

> ~~~
>
>
> 99.
> + <step>
> + <para>
> + Refresh the publications using
> + <link
> linkend="sql-altersubscription-params-refresh-publication"><command>ALTER
> SUBSCRIPTION ... REFRESH PUBLICATION</command></link>,
> + for e.g.:
>
> SUGGESTION
> Refresh the node2 subscription's publications using...

Modified

> ~~~
>
> 25.
> + <step>
> + <para>
> + Upgrade the node <literal>node3</literal>'s server to the required
> + new version, for e.g.:
>
> SUGGESTION
> Upgrade the node3 server to the required newer version, e.g.:

Modified

> ~~~
>
> 26.
> + <step>
> + <para>
> + Start the upgraded node <literal>node3</literal>'s server, for e.g.:
>
> SUGGESTION
> Start the upgraded node3's server, e.g.:

Modified

> ~~~
>
> 27.
> + <step>
> + <para>
> + Create any tables that were created in the upgraded node
> + <literal>node2</literal> between step-9 and now, for e.g.:
>
> 27a.
> SUGGESTION
> On node3, create any tables that were created in the upgraded node2 between...

Modified

> ~
>
> 27b.
> Maybe it is better to have a link to step9 instead of just hardwiring
> "Step-9" in the text.

Modified

> ~
>
> 27c.
> I didn't think it was needed to spread the CREATE TABLE across
> multiple lines. It is just a dummy example anyway so IMO better to use
> up less space.

Modified

> ~~~
>
> 28.
> + <step>
> + <para>
> + Refresh the publications using
> + <link
> linkend="sql-altersubscription-params-refresh-publication"><command>ALTER
> SUBSCRIPTION ... REFRESH PUBLICATION</command></link>,
> + for e.g.:
>
> SUGGESTION
> Refresh the node3 subscription's publications using...

Modified

> //////////
> Steps to Upgrade 2 node circular logical replication cluster</title>
> //////////
>
> (Again, some of these comments are similar to before, but I'll repeat
> them anyhow)
>
> ~~~
>
> 29. GENERAL - Should this circular scenario even be mentioned?
>
> IIUC there are no other PG docs for describing how to set up and
> manage a circular scenario like this. I know you wrote a blog about
> this topic [1], and I think there was a documentation patch [2] about
> this but it was never pushed.
>
> So, I'm not sure it is appropriate to include these docs "Steps to
> upgrade XXX" when there are not even any docs about "Steps to create
> XXX".

I feel we can add this later once this patch reaches a better shape

> ~~~
>
> 30.
> + <procedure>
> + <step>
> + <title>Steps to Upgrade 2 node circular logical replication
> cluster</title>
>
> SUGGESTION
> Steps to upgrade a two-node circular logical replication cluster

Modified

> ~~~
>
> 31.
> + <step>
> + <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>.
> + </para>
> + </step>
>
> 31a
> This renders as Step 1. But IMO this should not be a "step" at all --
> it's just a description of the scenario.
> REVIEW COMMENT 05/1

Modified

> ~
>
> 31b.
> The subsequent steps refer to subscriptions 'sub1_node1_node2' and
> 'sub2_node1_node2' and 'sub1_node2_node1' and 'sub1_node2_node1'. IMO
> it would help with the example code if those are named up front here
> too. e.g.
>
> node1 has two subscriptions for changes from node2:
> sub1_node2_node1
> sub2_node2_node1
>
> node2 has two subscriptions for changes from node1:
> sub1_node1_node2
> sub2_node1_node2

Modified

> ~~~
>
> 32.
> + <step>
> + <para>
> + Upgrade the node <literal>node1</literal>'s server to the required
> + newer version, for e.g.:
>
> SUGGESTION
> Upgrade the node1 server to the required newer version, e.g.:
>
> ~~~
>
> 33.
> + <step>
> + <para>
> + Start the upgraded node <literal>node1</literal>'s server, for e.g.:
>
> SUGGESTION
> Start the upgraded node1's server, e.g.:

Modified

> ~~~
>
> 34.
> + <step>
> + <para>
> + Wait till all the incremental changes are synchronized.
> + </para>
>
> Any hint on how to do this?

This is not required as it is already mentioned in the prerequisites
section. I have removed this.

> ~~~
>
> 35.
> + <step>
> + <para>
> + Create any tables that were created in <literal>node2</literal>
> + between step-2 and now, for e.g.:
>
> 35a.
> That doesn't seem right.
> - Don't you mean "created in the upgraded node1"?
> - Don't you mean "between step-5"?
>
> SUGGESTION
> On node2, create any tables that were created in the upgraded node1
> between step5 and...

This is correct, we need to create the tables since the subscription
was disabled

> ~
>
> 35b.
> Maybe it is better to have a link to step5 instead of just hardwiring
> "Step-5" in the text.

Modified

> ~
>
> 35c.
> I didn't think it was needed to spread the CREATE TABLE across
> multiple lines. It is just a dummy example anyway so IMO better to use
> up less space.

Modified

> ~~~
>
> 36.
> + <step>
> + <para>
> + Refresh the publications using
> + <link
> linkend="sql-altersubscription-params-refresh-publication"><command>ALTER
> SUBSCRIPTION ... REFRESH PUBLICATION</command></link>,
> + for e.g.:
>
> SUGGESTION
> Refresh the node2 subscription's publications using...

Modified

> ~~~
>
> 37.
> + <step>
> + <para>
> + Disable all the subscriptions on <literal>node1</literal> that are
> + subscribing the changes from <literal>node2</literal> by using
> + <link
> linkend="sql-altersubscription-params-disable"><command>ALTER
> SUBSCRIPTION ... DISABLE</command></link>,
> + for e.g.:
> +<programlisting>
> +node2=# ALTER SUBSCRIPTION sub1_node2_node1 DISABLE;
> +ALTER SUBSCRIPTION
> +node2=# ALTER SUBSCRIPTION sub2_node2_node1 DISABLE;
> +ALTER SUBSCRIPTION
> +</programlisting>
> + </para>
> + </step>
>
> This example looks wrong. IIUC these commands should be done on node1
> but the example shows a node2 prompt.

Modified

> ~~~
>
> 38.
> + <step>
> + <para>
> + Upgrade the node <literal>node2</literal>'s server to the required
> + new version, for e.g.:
>
> SUGGESTION
> Upgrade the node2 server to the required newer version, e.g.:

Modified

> ~~~
>
> 39.
> + <step>
> + <para>
> + Start the upgraded node <literal>node2</literal>'s server, for e.g.:
>
> SUGGESTION
> Start the upgraded node2's server, e.g.:

Modified

> ~~~
>
> 40.
> + <step>
> + <para>
> + Create any tables that were created in the upgraded node
> + <literal>node1</literal> between step-10 and now, for e.g.:
> +<programlisting>
> +node2=# CREATE TABLE distributors (
> +node2(# did integer CONSTRAINT no_null NOT NULL,
> +node2(# name varchar(40) NOT NULL
> +node2(# );
> +CREATE TABLE
> +</programlisting>
>
> 40a.
> That doesn't seem right.
> - Don't you mean "created in the upgraded node2"?
> - Don't you mean "between step-12"?
>
> SUGGESTION
> On node1, create any tables that were created in the upgraded node2
> between step12 and...

Modified

> ~
>
> 40b.
> Maybe it is better to have a link to step12 instead of just hardwiring
> "Step-12" in the text.

Modified

> ~
>
> 40c.
> I didn't think it was needed to spread the CREATE TABLE across
> multiple lines. It is just a dummy example anyway so IMO better to use
> up less space.

Modified

> ~~~
>
> 41.
> + <step>
> + <para>
> + Enable all the subscriptions on <literal>node1</literal> that are
> + subscribing the changes from <literal>node2</literal> by using
> + <link
> linkend="sql-altersubscription-params-enable"><command>ALTER
> SUBSCRIPTION ... ENABLE</command></link>,
> + for e.g.:
> +<programlisting>
> +node2=# ALTER SUBSCRIPTION sub1_node2_node1 ENABLE;
> +ALTER SUBSCRIPTION
> +node2=# ALTER SUBSCRIPTION sub2_node2_node1 ENABLE;
> +ALTER SUBSCRIPTION
> +</programlisting>
> + </para>
> + </step>
>
> The example looks wrong. IIUC these commands should be done on node1
> but the example shows a node2 prompt.

Modified

> ~~
>
> 42.
> + <step>
> + <para>
> + Refresh the publications using
> + <link
> linkend="sql-altersubscription-params-refresh-publication"><command>ALTER
> SUBSCRIPTION ... REFRESH PUBLICATION</command></link>
> + for e.g.:
> +<programlisting>
> +node2=# ALTER SUBSCRIPTION sub1_node1_node2 REFRESH PUBLICATION;
> +ALTER SUBSCRIPTION
> +node2=# ALTER SUBSCRIPTION sub2_node1_node2 REFRESH PUBLICATION;
> +ALTER SUBSCRIPTION
> +</programlisting>
> + </para>
> + </step>
>
> 42a.
> SUGGESTION
> Refresh the node1 subscription's publications using...

Modified

> ~
>
> 42b.
> The example looks wrong. IIUC these commands should be done on node1
> but the example shows a node2 prompt.

Modified

Thanks for the comments, the v2 version patch attached at [2] has the
fixes for the same.

[1] - https://www.postgresql.org/message-id/CAA4eK1KPFtxOzmkrJDY3LkeCkmWX5hZbSak7JLR57%2BvEq3afjQ%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CALDaNm2PD_eWLkLDs0qQ8MvWvh8j%3Dhee4_n6MX6Zz%3D%2BHosz%3Dpg%40mail.gmail.com

Regards,
Vignesh