Re: Documentation to upgrade logical replication cluster

On Mon, 15 Jan 2024 at 14:39, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Vignesh,
>
> Thanks for updating the patch!
>
> > > 7.
> > > ```
> > > +<programlisting>
> > > +dba(at)node1:/opt/PostgreSQL/postgres/&majorversion;/bin$ pg_ctl -D
> > /opt/PostgreSQL/pub_data stop -l logfile
> > > +</programlisting>
> > > ```
> > >
> > > Hmm. I thought you did not have to show the current directory. You were in the
> > > bin dir, but it is not our requirement, right?
> >
> > I kept this just to show the version being used
> >
>
> Hmm, but by default, the current directory is not set as PATH. So this example
> looks strange for me.

I have removed the paths shown to avoid confusion.

> Below lines are my comments for v2 patch.
>
> 01.
>
> ```
> + <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>
> ```
>
> I think we do not have to write it as one of steps. I think we can move to
> "Usage" part and modify like:
>
> This page only focus on nodes which are not logical replication participant. See
> <link linkend="logical-replication-upgrade"> for upgrading such nodes.

I have removed it from usage and moved it to the description section.

> 02.
>
> ```
> with the primary.) Only logical slots on the primary are copied to the
> new standby, but other slots on the old standby are not copied so must
> be recreated manually.
> ```
>
> A description for logical slots were remained. If you want to keep, we must
> say that it would be done for PG17+.

Mentioned as 17 or later.

> 03.
>
> I think the numbering seems bit confusing. sectX sgml tags should be used in
> this case. How about formatting like below?
>
> Upgrade (sect1)
> --- Prerequisites (sect2)
> --- For upgrading a publisher node (sect3)
> --- For upgrading a subscriber node (sect3)
> --- Examples (sect2)
> --- Two-node logical replication cluster (sect3)
> --- Cascaded logical replication cluster (sect3)
> --- Two-node circular logical replication cluster (sect3)

I felt this is better and changed it like:
30.11. Upgrade
--- 30.11.1. Prepare for publisher upgrades
--- 30.11.2. Prepare for subscriber upgrades
--- 30.11.3. Upgrading logical replication clusters
--- 30.11.3.1. Steps to upgrade a two-node logical replication cluster
--- 30.11.3.2. Steps to upgrade a cascaded logical replication cluster
--- 30.11.3.3. Steps to upgrade a two-node circular logical
replication cluster

> 04.
>
> Missing introduction in the head of this section. E.g.,
>
> Both publishers and subscribers can be upgraded, but there are some notes.
> Before reading this section, you should read <xref linkend="pgupgrade"/> page.

Added it with slight changes

> 05.
>
> ```
> + <step id="prepare-publisher-upgrades">
> + <title>Prepare for publisher upgrades</title>
> ...
> ```
>
> Should we describe in this page that publications can be upgraded in any
> versions?

I felt that need not be mentioned, as these are being upgraded from
earlier versions too

> 06.
>
> ```
> + <step id="prepare-subscriber-upgrades">
> + <title>Prepare for subscriber upgrades</title
> ```
>
> Same as 5, should we describe in this page that subscriptions can be upgraded
> in any versions?

I felt that need not be mentioned, as these are being upgraded from
earlier versions too

> 07.
>
> Basic considerations should be described before describing concrete steps.

The steps clearly mention the order in which it should be upgraded,
I'm not sure if we should repeat it again.

> E.g., publishers must be upgraded first. Also: While upgrading a subscriber,
> publisher can accept changes from users.

I have added this.

> 08.
>
> ```
> + two subscriptions sub1_node1_node2 and sub2_node1_node2 which is
> + subscribing the changes from <literal>node1</literal>.
> ```
>
> Both "sub1_node1_node2" and "sub2_node1_node2" must be rendered.

Modified

> 09.
>
> ```
> + <step>
> + <para>
> + Initialize data1_upgraded instance by using the required newer
> + version.
> + </para>
> ```
>
> Missing rendering. All similar paragraphs must be fixed.

Modified

> 10.
>
> ```
> + 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.:
> ```
>
> a.
>
> I think the link is not correct, it should refer Step 6. Can we add the step number?
> All similar paragraphs must be fixed.

I have kept it as step1 just in case any table is created before the
server is stopped in node1. So I felt it is better to refer to the
step of disabled subscription now.

> b.
>
> Not sure, but s/where disabled/were disabled/ ?
> All similar paragraphs must be fixed.

This is removed

> 11.
>
> ```
> + <para>
> + Refresh the <literal>node2</literal> subscription's publications using
> + <link linkend="sql-altersubscription-params-refresh-publication"><command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION</command></link>,
> + 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>
> ```
>
> Not sure, but should we clarify that copy_data must be on?

I have not mentioned here as copy_data by default is true in this case

> 12.
>
> ```
> + 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
> ```
>
> Name of subscriptions must be rendered.

Modified

> 13.
>
> ```
> + <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>
> ...
> + <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>
> ```
>
> Same tables were created, they must have another name.

For simplicity I used the same tables in all examples. I felt it should be ok

The v3 version patch attached at [1] has the changes for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm0ph5CFZ6ENL9EYiJhz3-xQMYx%2BUKWpFzggiLVfPKJoFw%40mail.gmail.com

Regards,
Vignesh