Re: Documentation to upgrade logical replication cluster

On Fri, 5 Jan 2024 at 09:08, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Vignesh,
>
> Thanks for making a patch! Below part is my comments.
>
> 1.
> Only two steps were added an id, but I think it should be for all the steps.
> See [1].

I have added wherever it is required as of now.

> 2.
> I'm not sure it should be listed as step 10. I felt that it should be new section.
> At that time other steps like "Prepare for {publisher|subscriber} upgrades" can be moved as well.
> Thought?

I have moved all of these to a separate page in logical-replication
under Upgrade

> 3.
> ```
> + The prerequisites of publisher upgrade applies to logical Replication
> ```
>
> Replication -> replication

Modified

> 4.
> ```
> + <para>
> + Let's say publisher is in <literal>node1</literal> and subscriber is
> + in <literal>node2</literal>.
> + </para>
> ```
>
> I felt it is more friendly if you added the name of directory for each instance.

I have listed this in the pg_upgrade command execution, since it is
mentioned there I have not added here too.

> 5.
> You did not write the initialization of new node. Was it intentional?

Added it now

> 6.
> ```
> + <para>
> + Disable all the subscriptions on <literal>node2</literal> that are
> + subscribing the changes from <literal>node1</literal> by using
> + <link linkend="sql-altersubscription-params-disable"><command>ALTER SUBSCRIPTION ... DISABLE</command></link>,
> + for e.g.:
> +<programlisting>
> +node2=# ALTER SUBSCRIPTION sub1_node1_node2 DISABLE;
> +ALTER SUBSCRIPTION
> +node2=# ALTER SUBSCRIPTION sub2_node1_node2 DISABLE;
> +ALTER SUBSCRIPTION
> +</programlisting>
> + </para>
> ```
>
> Subscriptions are disabled after stopping a publisher, but it leads ERRORs on the publisher.
> I think it's better to swap these steps.

Modified

> 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

> 8.
> ```
> +<programlisting>
> +dba(at)node1:/opt/PostgreSQL/postgres/&majorversion;/bin$ pg_upgrade
> + --old-datadir "/opt/PostgreSQL/postgres/17/pub_data"
> + --new-datadir "/opt/PostgreSQL/postgres/&majorversion;/pub_upgraded_data"
> + --old-bindir "/opt/PostgreSQL/postgres/17/bin"
> + --new-bindir "/opt/PostgreSQL/postgres/&majorversion;/bin"
> +</programlisting>
> ```
>
> For PG17, both old and new bindir look the same. Can we use 18 as new-bindir?

Modfied

> 9.
> ```
> + <para>
> + Create any tables that were created in <literal>node2</literal>
> + between step-2 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>
> ```
>
> I think this SQLs must be done on node1, because it has not boot between step-2
> and step-7.

Modified

> 10.
> ```
> + <step>
> + <para>
> + Enable all the subscriptions on <literal>node2</literal> that are
> + subscribing the changes from <literal>node1</literal> by using
> + <link linkend="sql-altersubscription-params-enable"><command>ALTER SUBSCRIPTION ... ENABLE</command></link>,
> + for e.g.:
> +<programlisting>
> +node2=# ALTER SUBSCRIPTION sub1_node1_node2 ENABLE;
> +ALTER SUBSCRIPTION
> +node2=# ALTER SUBSCRIPTION sub2_node1_node2 ENABLE;
> +ALTER SUBSCRIPTION
> +</programlisting>
> + </para>
> + </step>
> +
> + <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>
> ```
>
> I was very confused the location where they would be really do. If my above
> comment is correct, should they be executed on node1 as well? Could you please all
> the notation again?

Modified

> 11.
> ```
> + <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>
> ```
>
> They should be on node1, but noted as node2.

Modified

> 12.
> ```
> + <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>
> ```
>
> You said that "enable all the subscription on node1", but SQLs are done on node2.

Modified

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

Regards,
Vignesh