RE: Documentation to upgrade logical replication cluster - Mailing list pgsql-hackers

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

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?

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

Replication -> replication

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.

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

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.

7.
```
+<programlisting>
+dba@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? 

8.
```
+<programlisting>
+dba@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?

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.

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?

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.

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.

[1]:
https://www.postgresql.org/message-id/TYAPR01MB58667AE04D291924671E2051F5879@TYAPR01MB5866.jpnprd01.prod.outlook.com


Best Regards,
Hayato Kuroda
FUJITSU LIMITED