RE: Documentation to upgrade logical replication cluster

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.

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.

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

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)

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.

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?

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?

07.

Basic considerations should be described before describing concrete steps.
E.g., publishers must be upgraded first. Also: While upgrading a subscriber,
publisher can accept changes from users.

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.

09.

```
+ <step>
+ <para>
+ Initialize data1_upgraded instance by using the required newer
+ version.
+ </para>
```

Missing rendering. All similar paragraphs must be fixed.

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.

b.

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

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?

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.

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.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED