Обсуждение: Documentation to upgrade logical replication cluster

Hi,

We have documentation on how to upgrade "publisher" and "subscriber"
at [1], but currently we do not have any documentation on how to
upgrade logical replication clusters.
Here is a patch to document how to upgrade different logical
replication clusters: a) Upgrade 2 node logical replication cluster b)
Upgrade cascaded logical replication cluster c) Upgrade 2 node
circular logical replication cluster.
Thoughts?

[1] - https://www.postgresql.org/docs/devel/pgupgrade.html

Regards,
Vignesh

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

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.

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.

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

~~~

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/

~~~

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/

~~~

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/

~~~

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.

~~~

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

~~~

9.
+     <step>
+      <title>Steps to Upgrade 2 node logical replication cluster</title>

SUGGESTION
Steps to upgrade a two-node logical replication cluster

~~~

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.

~

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

~~~

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

~~~

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

~~~

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

~~~

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

~~~

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.

~

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.

~~~

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/

~~~

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

~

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

~~~

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

~~~

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

SUGGESTION
Start the upgraded node1's server, e.g.:

~~~

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

~~~

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

SUGGESTION
Start the upgraded node2's server, e.g.:

~~~

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

~

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

~

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.

~~~

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/

~~~


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

~~~

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

~~~

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

SUGGESTION
Start the upgraded node3's server, e.g.:

~~~

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

~

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

~

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.

~~~

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

//////////
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".

~~~

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

~~~

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

~

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

~~~

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

~~~

34.
+        <step>
+         <para>
+          Wait till all the incremental changes are synchronized.
+         </para>

Any hint on how to do 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...

~

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

~

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.

~~~

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

~~~

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.

~~~

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

~~~

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

SUGGESTION
Start the upgraded node2's server, e.g.:

~~~

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

~

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

~

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.

~~~

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.

~~

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

~

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

======
[1] https://www.postgresql.fastware.com/blog/bi-directional-replication-using-origin-filtering-in-postgresql
[2] https://www.postgresql.org/message-id/CALDaNm3tv%2BnWMXO0q39EuwzbXEQyF5thT4Ha1PvfQ%2BfQgSdi_A%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Jan 5, 2024 at 2:38 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
...
> 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?

During my review, I also felt that step 10 is now so long that it is a
distraction from the other content on this page.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Jan 4, 2024 at 2:22 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> We have documentation on how to upgrade "publisher" and "subscriber"
> at [1], but currently we do not have any documentation on how to
> upgrade logical replication clusters.
> Here is a patch to document how to upgrade different logical
> replication clusters: a) Upgrade 2 node logical replication cluster b)
> Upgrade cascaded logical replication cluster c) Upgrade 2 node
> circular logical replication cluster.
> Thoughts?
>
> [1] - https://www.postgresql.org/docs/devel/pgupgrade.html

Thanks for this. It really helps developers a lot. In addition to the
docs, why can't all of these steps be put into a perl/shell script or
a C tool sitting in the src/bin directory?

I prefer a postgres src/bin tool which takes publisher and subscriber
connection strings as the inputs, talks to them and upgrades both
publisher and subscriber. Of course, one can write such a tool outside
of postgres in their own programming language, but the capability to
upgrade postgres servers with logical replication is such an important
task one would often require it. Therefore, an off-the-shelf tool not
only avoids manual efforts but makes it effortless for the users,
after all, if any of the steps isn't performed as stated in the docs
the servers may end up in an inconsistent state.

Thoughts?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Thu, Jan 4, 2024 at 2:22 PM vignesh C <vignesh21@gmail.com> wrote:
>
> We have documentation on how to upgrade "publisher" and "subscriber"
> at [1], but currently we do not have any documentation on how to
> upgrade logical replication clusters.
> Here is a patch to document how to upgrade different logical
> replication clusters: a) Upgrade 2 node logical replication cluster b)
> Upgrade cascaded logical replication cluster c) Upgrade 2 node
> circular logical replication cluster.
>

Today, off-list, I had a short discussion on this documentation with
Jonathan and Michael. I was not sure whether we should add this in the
main documentation of the upgrade or maintain it as a separate wiki
page. My primary worry was that this seemed to be taking too much
space on pgupgrade page and making the information on that page a bit
unreadable. Jonathan suggested that we can add this information to the
logical replication page [1] and add a reference in the pgupgrade
page. That suggestion makes sense to me considering we have
sub-sections like Monitoring, Security, and Configuration Settings on
the logical replication page. We can have a new sub-section Upgrade on
the same lines. What do you think?

[1] - https://www.postgresql.org/docs/devel/logical-replication.html

--
With Regards,
Amit Kapila.

On Mon, Jan 8, 2024 at 12:52 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Jan 4, 2024 at 2:22 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Hi,
> >
> > We have documentation on how to upgrade "publisher" and "subscriber"
> > at [1], but currently we do not have any documentation on how to
> > upgrade logical replication clusters.
> > Here is a patch to document how to upgrade different logical
> > replication clusters: a) Upgrade 2 node logical replication cluster b)
> > Upgrade cascaded logical replication cluster c) Upgrade 2 node
> > circular logical replication cluster.
> > Thoughts?
> >
> > [1] - https://www.postgresql.org/docs/devel/pgupgrade.html
>
> Thanks for this. It really helps developers a lot. In addition to the
> docs, why can't all of these steps be put into a perl/shell script or
> a C tool sitting in the src/bin directory?
>
> I prefer a postgres src/bin tool which takes publisher and subscriber
> connection strings as the inputs, talks to them and upgrades both
> publisher and subscriber. Of course, one can write such a tool outside
> of postgres in their own programming language, but the capability to
> upgrade postgres servers with logical replication is such an important
> task one would often require it. Therefore, an off-the-shelf tool not
> only avoids manual efforts but makes it effortless for the users,
> after all, if any of the steps isn't performed as stated in the docs
> the servers may end up in an inconsistent state.
>

This idea has merits but not sure if we just add a few tests that
users can refer to if they want or provide a utility as you described.
I would prefer a test or two for now and if there is a demand then we
can consider having such a utility. In either case, I feel it is
better discussed in a separate thread.

--
With Regards,
Amit Kapila.

On Wed, 10 Jan 2024 at 15:50, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jan 4, 2024 at 2:22 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > We have documentation on how to upgrade "publisher" and "subscriber"
> > at [1], but currently we do not have any documentation on how to
> > upgrade logical replication clusters.
> > Here is a patch to document how to upgrade different logical
> > replication clusters: a) Upgrade 2 node logical replication cluster b)
> > Upgrade cascaded logical replication cluster c) Upgrade 2 node
> > circular logical replication cluster.
> >
>
> Today, off-list, I had a short discussion on this documentation with
> Jonathan and Michael. I was not sure whether we should add this in the
> main documentation of the upgrade or maintain it as a separate wiki
> page. My primary worry was that this seemed to be taking too much
> space on pgupgrade page and making the information on that page a bit
> unreadable. Jonathan suggested that we can add this information to the
> logical replication page [1] and add a reference in the pgupgrade
> page. That suggestion makes sense to me considering we have
> sub-sections like Monitoring, Security, and Configuration Settings on
> the logical replication page. We can have a new sub-section Upgrade on
> the same lines. What do you think?

I feel that would be better, also others like Kuroda-san had said in
the similar lines at comment-2 at [1] and Peter also had similar
opinion at [2]. I will handle this in the next version.

[1] -
https://www.postgresql.org/message-id/TY3PR01MB9889BD1202530E8310AC9B3DF5662%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[2] -
https://www.postgresql.org/message-id/CAHut%2BPs4AtGB9MMK51%3D1Z1JQ1FUK%2BX0oXQuAdEad1kEEuw7%2BkA%40mail.gmail.com

Regards,
Vignesh

On Fri, 5 Jan 2024 at 09:08, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.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@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@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

On Fri, 5 Jan 2024 at 10:49, Peter Smith <smithpb2250@gmail.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

Hi Vignesh, here are some review comments for patch v2-0001.

======
doc/src/sgml/ref/pgupgrade.sgml

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

This renders like:
Refer logical replication upgrade section for details on upgrading
logical replication clusters.

~

IMO it would be better to use xref instead of link, which will render
more normally like:
See Section 30.11 for details on upgrading logical replication clusters.

SUGGESTION
    <para>
     See <xref linkend="logical-replication-upgrade"/>
     for details on upgrading logical replication clusters.
    </para>

======
doc/src/sgml/logical-replication.sgml

2. GENERAL - blurb

+ <sect1 id="logical-replication-upgrade">
+  <title>Upgrade</title>
+
+  <procedure>
+   <step id="prepare-publisher-upgrades">
+    <title>Prepare for publisher upgrades</title>

I felt there should be a short (1 or 2 sentence) general blurb about
pub/sub upgrade before jumping straight into:

"1. Prepare for publisher upgrades"
"2. Prepare for subscriber upgrades"
"3. Upgrading logical replication cluster"

~

Specifically, at first, it looks strange that the HTML renders as
steps 1,2,3 instead of sub-sections (30.11.1, 30.11.2, 30.11.3); Maybe
"steps" are fine, but then at least there needs to be some intro
sentence saying like "follow these steps:"

~~~

3.
+   <step id="upgrading-logical-replication-cluster">
+    <title>Upgrading logical replication cluster</title>

/cluster/clusters/

~~~

4.
+    <para>
+     The steps to upgrade the following logical replication clusters are
+     detailed below:
+     <itemizedlist>
+      <listitem>
+       <para>
+        <link linkend="steps-two-node-logical-replication-cluster">Two-node
logical replication cluster.</link>
+       </para>
+      </listitem>
+      <listitem>
+       <para>
+        <link linkend="steps-cascaded-logical-replication-cluster">Cascaded
logical replication cluster.</link>
+       </para>
+      </listitem>
+      <listitem>
+       <para>
+        <link linkend="steps-two-node-circular-logical-replication-cluster">Two-node
circular logical replication cluster.</link>
+       </para>
+      </listitem>
+     </itemizedlist>
+    </para>

Isn't there a better way to accomplish this by using xref and
'xreflabel' so you don't have to type the link text here?


//////////
Steps to upgrade a two-node logical replication cluster
//////////

5.
+      <para>
+       Let's say publisher is in <literal>node1</literal> and subscriber is
+       in <literal>node2</literal>. The subscriber <literal>node2</literal> has
+       two subscriptions sub1_node1_node2 and sub2_node1_node2 which is
+       subscribing the changes from <literal>node1</literal>.
+      </para>

5a
Those subscription names should also be rendered as literals.

~

5b
/which is/which are/

~~~

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

data1_upgraded should be rendered as literal.

~~~

7.
+
+       <step>
+        <para>
+         Initialize data2_upgraded instance by using the required newer
+         version.
+        </para>
+       </step>

data2_upgraded should be rendered as literal.

~~~

8.
+
+       <step>
+        <para>
+         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.:
+<programlisting>
+node2=# CREATE TABLE distributors (did integer PRIMARY KEY, name varchar(40));
+CREATE TABLE
+</programlisting>
+        </para>
+       </step>

8a.
This link to the earlier step renders badly like:
On node2, create any tables that were created in the upgraded
publisher node1 server between when the subscriptions where disabled
in node2 and now, e.g.:

IMO this link should be like "Step N", not some words -- maybe it is
another opportunity for using xreflabel?

~

8b.
Also has typos "when the subscriptions where disabled" (??)

//////////
Steps to upgrade a cascaded logical replication clusters
//////////

9.
+    <procedure>
+     <step id="steps-cascaded-logical-replication-cluster">
+      <title>Steps to upgrade a cascaded logical replication clusters</title>

The title has a strange mix of singular "a" and plural "clusters"

~~~

10.
+      <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>. The <literal>node2</literal>
+       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
+       <literal>node2</literal>.
+      </para>

10a.
Those subscription names should also be rendered as literals.

~

10b.
/which is/which are/ (occurs 2x)

~~~

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

data1_upgraded should be rendered as literal.

~~~

12.
+
+       <step>
+        <para>
+         Initialize data2_upgraded instance by using the required
newer version.
+        </para>
+       </step>

data2_upgraded should be rendered as literal.

~~~

13.
+
+       <step>
+        <para>
+         On <literal>node2</literal>, create any tables that were created in
+         the upgraded publisher <literal>node1</literal> server between
+         <link linkend="cascaded-cluster-disable-sub-node1-node2">
+         when the subscriptions where disabled in
<literal>node2</literal></link>
+         and now, e.g.:
+<programlisting>
+node2=# CREATE TABLE distributors (did integer PRIMARY KEY, name varchar(40));
+CREATE TABLE
+</programlisting>
+        </para>
+       </step>

13a.
This link to the earlier step renders badly like:
On node2, create any tables that were created in the upgraded
publisher node1 server between when the subscriptions where disabled
in node2 and now, e.g.:

IMO this link should be like "Step N", not some words -- maybe it is
another opportunity for using xreflabel?

~

13b
Also has typos "when the subscriptions where disabled" (??)

~~~

14.
+
+       <step>
+        <para>
+         Initialize data3_upgraded instance by using the required
newer version.
+       </para>
+       </step>

data3_upgraded should be rendered as literal.

~~~

15.
+
+       <step>
+        <para>
+         On <literal>node3</literal>, create any tables that were created in
+         the upgraded <literal>node2</literal> between
+         <link linkend="cascaded-cluster-disable-sub-node2-node3">when the
+         subscriptions where disabled in <literal>node3</literal></link>
+         and now, e.g.:
+<programlisting>
+node3=# CREATE TABLE distributors (did integer PRIMARY KEY, name varchar(40));
+CREATE TABLE
+</programlisting>
+        </para>
+       </step>

15a.
This link to the earlier step renders badly like:
On node3, create any tables that were created in the upgraded node2
between when the subscriptions where disabled in node3 and now, e.g.:

~

15b.
Also has typos "when the subscriptions where disabled" (??)

//////////
Steps to upgrade a two-node circular logical replication cluster
//////////

16.
+      <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>. The <literal>node1</literal>
+       has two subscriptions sub1_node2_node1 and sub2_node2_node1 which is
+       subscribing the changes from <literal>node2</literal>. The
+       <literal>node2</literal> has two subscriptions sub1_node1_node2 and
+       sub2_node1_node2 which is subscribing the changes from
+       <literal>node1</literal>.
+      </para>

16a
Those subscription names should also be rendered as literals.

~

16b
/which is/which are/

~~~

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

data1_upgraded should render as literal.

~~~

18.
+
+       <step>
+        <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>
+       </step>

18a.
This link to the earlier step renders badly like:
On node1, Create any tables that were created in node2 between when
the subscriptions where disabled in node2 and now, e.g.:

IMO this link should be like "Step N", not some words -- maybe it is
another opportunity for using xreflabel?
~

18b
Also has typos "when the subscriptions where disabled" (??)

~

18c.
/Create any/create any/

~~~

19.
+
+       <step>
+        <para>
+         Initialize data2_upgraded instance by using the required newer
+         version.
+        </para>
+       </step>

data2_upgraded should render as literal.

~~~

20.
+
+       <step>
+        <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>
+       </step>

20a.
This link to the earlier step renders badly like:
On node2, Create any tables that were created in the upgraded node1
between when the subscriptions where disabled in node1 and now, e.g.:

~

20b
Also has typos "when the subscriptions where disabled" (??)

~

20c.
/Create any/create any/

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Dear Vignesh,

Thanks for updating the patch!

> > 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?
> 
> 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

On Mon, 15 Jan 2024 at 09:01, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh, here are some review comments for patch v2-0001.
>
> ======
> doc/src/sgml/ref/pgupgrade.sgml
>
> 1.
> +   <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>
> +
>
> This renders like:
> Refer logical replication upgrade section for details on upgrading
> logical replication clusters.
>
> ~
>
> IMO it would be better to use xref instead of link, which will render
> more normally like:
> See Section 30.11 for details on upgrading logical replication clusters.
>
> SUGGESTION
>     <para>
>      See <xref linkend="logical-replication-upgrade"/>
>      for details on upgrading logical replication clusters.
>     </para>

Modified

> ======
> doc/src/sgml/logical-replication.sgml
>
> 2. GENERAL - blurb
>
> + <sect1 id="logical-replication-upgrade">
> +  <title>Upgrade</title>
> +
> +  <procedure>
> +   <step id="prepare-publisher-upgrades">
> +    <title>Prepare for publisher upgrades</title>
>
> I felt there should be a short (1 or 2 sentence) general blurb about
> pub/sub upgrade before jumping straight into:
>
> "1. Prepare for publisher upgrades"
> "2. Prepare for subscriber upgrades"
> "3. Upgrading logical replication cluster"

Added

> ~
>
> Specifically, at first, it looks strange that the HTML renders as
> steps 1,2,3 instead of sub-sections (30.11.1, 30.11.2, 30.11.3); Maybe
> "steps" are fine, but then at least there needs to be some intro
> sentence saying like "follow these steps:"
> ~~~

Modified

>
> 3.
> +   <step id="upgrading-logical-replication-cluster">
> +    <title>Upgrading logical replication cluster</title>
>
> /cluster/clusters/

Modified

> ~~~
>
> 4.
> +    <para>
> +     The steps to upgrade the following logical replication clusters are
> +     detailed below:
> +     <itemizedlist>
> +      <listitem>
> +       <para>
> +        <link linkend="steps-two-node-logical-replication-cluster">Two-node
> logical replication cluster.</link>
> +       </para>
> +      </listitem>
> +      <listitem>
> +       <para>
> +        <link linkend="steps-cascaded-logical-replication-cluster">Cascaded
> logical replication cluster.</link>
> +       </para>
> +      </listitem>
> +      <listitem>
> +       <para>
> +        <link linkend="steps-two-node-circular-logical-replication-cluster">Two-node
> circular logical replication cluster.</link>
> +       </para>
> +      </listitem>
> +     </itemizedlist>
> +    </para>
>
> Isn't there a better way to accomplish this by using xref and
> 'xreflabel' so you don't have to type the link text here?

Modified

>
> //////////
> Steps to upgrade a two-node logical replication cluster
> //////////
>
> 5.
> +      <para>
> +       Let's say publisher is in <literal>node1</literal> and subscriber is
> +       in <literal>node2</literal>. The subscriber <literal>node2</literal> has
> +       two subscriptions sub1_node1_node2 and sub2_node1_node2 which is
> +       subscribing the changes from <literal>node1</literal>.
> +      </para>
>
> 5a
> Those subscription names should also be rendered as literals.

Modified

> ~
>
> 5b
> /which is/which are/

Modified

> ~~~
>
> 6.
> +       <step>
> +        <para>
> +         Initialize data1_upgraded instance by using the required newer
> +         version.
> +        </para>
> +       </step>
>
> data1_upgraded should be rendered as literal.

Modified

> ~~~
>
> 7.
> +
> +       <step>
> +        <para>
> +         Initialize data2_upgraded instance by using the required newer
> +         version.
> +        </para>
> +       </step>
>
> data2_upgraded should be rendered as literal.

Modified

> ~~~
>
> 8.
> +
> +       <step>
> +        <para>
> +         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.:
> +<programlisting>
> +node2=# CREATE TABLE distributors (did integer PRIMARY KEY, name varchar(40));
> +CREATE TABLE
> +</programlisting>
> +        </para>
> +       </step>
>
> 8a.
> This link to the earlier step renders badly like:
> On node2, create any tables that were created in the upgraded
> publisher node1 server between when the subscriptions where disabled
> in node2 and now, e.g.:
>
> IMO this link should be like "Step N", not some words -- maybe it is
> another opportunity for using xreflabel?

Modified

> ~
>
> 8b.
> Also has typos "when the subscriptions where disabled" (??)

This is not required after using xref, removed it.

> //////////
> Steps to upgrade a cascaded logical replication clusters
> //////////
>
> 9.
> +    <procedure>
> +     <step id="steps-cascaded-logical-replication-cluster">
> +      <title>Steps to upgrade a cascaded logical replication clusters</title>
>
> The title has a strange mix of singular "a" and plural "clusters"

Changed it to keep it consistent

> ~~~
>
> 10.
> +      <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>. The <literal>node2</literal>
> +       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
> +       <literal>node2</literal>.
> +      </para>
>
> 10a.
> Those subscription names should also be rendered as literals.

Modified

> ~
>
> 10b.
> /which is/which are/ (occurs 2x)

Modified

> ~~~
>
> 11.
> +
> +       <step>
> +        <para>
> +         Initialize data1_upgraded instance by using the required
> newer version.
> +        </para>
> +       </step>
>
> data1_upgraded should be rendered as literal.

Modified

> ~~~
>
> 12.
> +
> +       <step>
> +        <para>
> +         Initialize data2_upgraded instance by using the required
> newer version.
> +        </para>
> +       </step>
>
> data2_upgraded should be rendered as literal.

Modified

> ~~~
>
> 13.
> +
> +       <step>
> +        <para>
> +         On <literal>node2</literal>, create any tables that were created in
> +         the upgraded publisher <literal>node1</literal> server between
> +         <link linkend="cascaded-cluster-disable-sub-node1-node2">
> +         when the subscriptions where disabled in
> <literal>node2</literal></link>
> +         and now, e.g.:
> +<programlisting>
> +node2=# CREATE TABLE distributors (did integer PRIMARY KEY, name varchar(40));
> +CREATE TABLE
> +</programlisting>
> +        </para>
> +       </step>
>
> 13a.
> This link to the earlier step renders badly like:
> On node2, create any tables that were created in the upgraded
> publisher node1 server between when the subscriptions where disabled
> in node2 and now, e.g.:
>
> IMO this link should be like "Step N", not some words -- maybe it is
> another opportunity for using xreflabel?

Modified

> ~
>
> 13b
> Also has typos "when the subscriptions where disabled" (??)

This is not required after using xref, removed it.

> ~~~
>
> 14.
> +
> +       <step>
> +        <para>
> +         Initialize data3_upgraded instance by using the required
> newer version.
> +       </para>
> +       </step>
>
> data3_upgraded should be rendered as literal.

Modified

> ~~~
>
> 15.
> +
> +       <step>
> +        <para>
> +         On <literal>node3</literal>, create any tables that were created in
> +         the upgraded <literal>node2</literal> between
> +         <link linkend="cascaded-cluster-disable-sub-node2-node3">when the
> +         subscriptions where disabled in <literal>node3</literal></link>
> +         and now, e.g.:
> +<programlisting>
> +node3=# CREATE TABLE distributors (did integer PRIMARY KEY, name varchar(40));
> +CREATE TABLE
> +</programlisting>
> +        </para>
> +       </step>
>
> 15a.
> This link to the earlier step renders badly like:
> On node3, create any tables that were created in the upgraded node2
> between when the subscriptions where disabled in node3 and now, e.g.:

Changed it to xref.

> ~
>
> 15b.
> Also has typos "when the subscriptions where disabled" (??)

This is not required after using xref, removed it.

> //////////
> Steps to upgrade a two-node circular logical replication cluster
> //////////
>
> 16.
> +      <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>. The <literal>node1</literal>
> +       has two subscriptions sub1_node2_node1 and sub2_node2_node1 which is
> +       subscribing the changes from <literal>node2</literal>. The
> +       <literal>node2</literal> has two subscriptions sub1_node1_node2 and
> +       sub2_node1_node2 which is subscribing the changes from
> +       <literal>node1</literal>.
> +      </para>
>
> 16a
> Those subscription names should also be rendered as literals.

Modified

> ~
>
> 16b
> /which is/which are/

Modified

> ~~~
>
> 17.
> +
> +       <step>
> +        <para>
> +         Initialize data1_upgraded instance by using the required newer
> +         version.
> +        </para>
> +       </step>
>
> data1_upgraded should render as literal.

Modified

> ~~~
>
> 18.
> +
> +       <step>
> +        <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>
> +       </step>
>
> 18a.
> This link to the earlier step renders badly like:
> On node1, Create any tables that were created in node2 between when
> the subscriptions where disabled in node2 and now, e.g.:
>
> IMO this link should be like "Step N", not some words -- maybe it is
> another opportunity for using xreflabel?

Modified to xref

>
> 18b
> Also has typos "when the subscriptions where disabled" (??)

This is not required after using xref, removed it.

> ~
>
> 18c.
> /Create any/create any/

Modified

> ~~~
>
> 19.
> +
> +       <step>
> +        <para>
> +         Initialize data2_upgraded instance by using the required newer
> +         version.
> +        </para>
> +       </step>
>
> data2_upgraded should render as literal.

Modified

> ~~~
>
> 20.
> +
> +       <step>
> +        <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>
> +       </step>
>
> 20a.
> This link to the earlier step renders badly like:
> On node2, Create any tables that were created in the upgraded node1
> between when the subscriptions where disabled in node1 and now, e.g.:

Modified to xref

> ~
>
> 20b
> Also has typos "when the subscriptions where disabled" (??)

This is not required after using xref, removed it.

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

Regards,
Vignesh

On Mon, 15 Jan 2024 at 14:39, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> Thanks for updating the patch!
>
> > > 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?
> >
> > 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

Dear Vignesh,

Thanks for updating the patch! Basically your patch looks good.
Below lines are my comments for v3.

01.

```
     <para>
      The output plugins referenced by the slots on the old cluster must be
      installed in the new PostgreSQL executable directory.
     </para>
```

PostgreSQL must be marked as <productname>.

02.

```
<programlisting>
pg_ctl -D /opt/PostgreSQL/data1 stop -l logfile
</programlisting>
```

I checked that found that -l was no-op when `pg_ctl stop` was specified. Can we remove?
The documentation is not listed -l for the stop command.
All the similar lines should be fixed as well.

03.

```
        On <literal>node3</literal>, create any tables that were created in
        the upgraded <literal>node2</literal> between
        <xref linkend="cascaded-cluster-disable-sub-node2-node3"/> and now,
```

If tables are newly defined on node1 between 1 - 11, they are not defined on node3.
So they must be defined on node3 as well.

04.

```
      <step>
       <para id="cascaded-cluster-disable-sub-node2-node3">
```

Even if the referred steps is correct, ID should be allocated to step, not para.
That's why the rendering is bit a strange.


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Here are some review comments for patch v3.

======
doc/src/sgml/ref/pgupgrade.sgml

1.
+
+  <para>
+   This page does not cover steps to upgrade logical replication
clusters, refer
+   <xref linkend="logical-replication-upgrade"/> for details on upgrading
+   logical replication clusters.
+  </para>
+

I felt that maybe this note was misplaced. Won't it be better to put
this down in the "Usage" section of this page?

BEFORE
These are the steps to perform an upgrade with pg_upgrade:

SUGGESTION (or something like this)
Below are the steps to perform an upgrade with pg_upgrade.

Note, the steps to upgrade logical replication clusters are not
covered here; refer to <xref linkend="logical-replication-upgrade"/>
for details.

~~~

2.
        Configure the servers for log shipping.  (You do not need to run
        <function>pg_backup_start()</function> and
<function>pg_backup_stop()</function>
        or take a file system backup as the standbys are still synchronized
-       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.
+       with the primary.)  In version 17.0 or later, 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.
       </para>

This para was still unclear to me. What is version 17.0 referring to
-- the old_cluster version? Do you mean something like:
If the old cluster is < v17 then logical slots are not copied. If the
old_cluster is >= v17 then...

======
doc/src/sgml/logical-replication.sgml

3.
+   <para>
+    While upgrading a subscriber, write operations can be performed in the
+    publisher, these changes will be replicated to the subscriber once the
+    subscriber upgradation is completed.
+   </para>

3a.
/publisher, these changes/publisher. These changes/

~

3b.
"upgradation" ??. See [1]

maybe just /upgradation/upgrade/

~~~

4. GENERAL - prompts/paths

I noticed in v3 you removed all the cmd prompts like:
dba@node1:/opt/PostgreSQL/postgres/17/bin$
dba@node2:/opt/PostgreSQL/postgres/18/bin$
etc.

I thought those were helpful to disambiguate which server/version was
being operated on. I wonder if there is some way to keep information
still but not make it look like a real current directory that
Kuroda-san did not like:

e.g. Maybe something like the below is possible?

(dba@node1: v17) pg_upgrade...
(dba@node2: v18) pg_upgrade...

======
[1]
https://english.stackexchange.com/questions/192187/upgradation-not-universally-accepted#:~:text=Not%20all%20dictionaries%20(or%20native,by%20most%20non%2DIE%20speakers.

Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, 24 Jan 2024 at 15:16, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> Thanks for updating the patch! Basically your patch looks good.
> Below lines are my comments for v3.
>
> 01.
>
> ```
>      <para>
>       The output plugins referenced by the slots on the old cluster must be
>       installed in the new PostgreSQL executable directory.
>      </para>
> ```
>
> PostgreSQL must be marked as <productname>.

Modified

> 02.
>
> ```
> <programlisting>
> pg_ctl -D /opt/PostgreSQL/data1 stop -l logfile
> </programlisting>
> ```
>
> I checked that found that -l was no-op when `pg_ctl stop` was specified. Can we remove?
> The documentation is not listed -l for the stop command.
> All the similar lines should be fixed as well.

Modified

> 03.
>
> ```
>         On <literal>node3</literal>, create any tables that were created in
>         the upgraded <literal>node2</literal> between
>         <xref linkend="cascaded-cluster-disable-sub-node2-node3"/> and now,
> ```
>
> If tables are newly defined on node1 between 1 - 11, they are not defined on node3.
> So they must be defined on node3 as well.

The new node1 tables will be created in node2 in step-11. Since we
have mentioned that, create the tables that were created between step
6 and now, all of node1 and node2 tables will get created

> 04.
>
> ```
>       <step>
>        <para id="cascaded-cluster-disable-sub-node2-node3">
> ```
>
> Even if the referred steps is correct, ID should be allocated to step, not para.
> That's why the rendering is bit a strange.

Modified

The attached v4 version patch has the changes for the same.

Regards,
Vignesh

On Thu, 25 Jan 2024 at 05:45, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for patch v3.
>
> ======
> doc/src/sgml/ref/pgupgrade.sgml
>
> 1.
> +
> +  <para>
> +   This page does not cover steps to upgrade logical replication
> clusters, refer
> +   <xref linkend="logical-replication-upgrade"/> for details on upgrading
> +   logical replication clusters.
> +  </para>
> +
>
> I felt that maybe this note was misplaced. Won't it be better to put
> this down in the "Usage" section of this page?
>
> BEFORE
> These are the steps to perform an upgrade with pg_upgrade:
>
> SUGGESTION (or something like this)
> Below are the steps to perform an upgrade with pg_upgrade.
>
> Note, the steps to upgrade logical replication clusters are not
> covered here; refer to <xref linkend="logical-replication-upgrade"/>
> for details.

Modified

> ~~~
>
> 2.
>         Configure the servers for log shipping.  (You do not need to run
>         <function>pg_backup_start()</function> and
> <function>pg_backup_stop()</function>
>         or take a file system backup as the standbys are still synchronized
> -       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.
> +       with the primary.)  In version 17.0 or later, 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.
>        </para>
>
> This para was still unclear to me. What is version 17.0 referring to
> -- the old_cluster version? Do you mean something like:
> If the old cluster is < v17 then logical slots are not copied. If the
> old_cluster is >= v17 then...

Yes, I have rephrased it now.

> ======
> doc/src/sgml/logical-replication.sgml
>
> 3.
> +   <para>
> +    While upgrading a subscriber, write operations can be performed in the
> +    publisher, these changes will be replicated to the subscriber once the
> +    subscriber upgradation is completed.
> +   </para>
>
> 3a.
> /publisher, these changes/publisher. These changes/

Modified

> ~
>
> 3b.
> "upgradation" ??. See [1]
>
> maybe just /upgradation/upgrade/

Modified

> ~~~
>
> 4. GENERAL - prompts/paths
>
> I noticed in v3 you removed all the cmd prompts like:
> dba@node1:/opt/PostgreSQL/postgres/17/bin$
> dba@node2:/opt/PostgreSQL/postgres/18/bin$
> etc.
>
> I thought those were helpful to disambiguate which server/version was
> being operated on. I wonder if there is some way to keep information
> still but not make it look like a real current directory that
> Kuroda-san did not like:
>
> e.g. Maybe something like the below is possible?
>
> (dba@node1: v17) pg_upgrade...
> (dba@node2: v18) pg_upgrade...

I did not want to add this as our current documentation is consistent
with how it is documented in the pg_upgrade page at [1].

The v4 version patch attached at [2] has the changes for the same.

[1] - https://www.postgresql.org/docs/devel/pgupgrade.html
[2] - https://www.postgresql.org/message-id/CALDaNm1wCHmBwpLM%3Dd9oBoZqKXOe-TwC-LCcHC9gFy0bazZU6Q%40mail.gmail.com

Regards,
Vignesh

Hi Vignesh,

Here are some review comments for patch v4.

These are cosmetic only; otherwise v4 LGTM.

======
doc/src/sgml/ref/pgupgrade.sgml

1.
        Configure the servers for log shipping.  (You do not need to run
        <function>pg_backup_start()</function> and
<function>pg_backup_stop()</function>
        or take a file system backup as the standbys are still synchronized
-       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.
+       with the primary.)  If the old cluster is prior to 17.0, then no slots
+       on the primary are copied to the new standby, so all the slots must be
+       recreated manually. If the old cluster is 17.0 or later, then 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.
       </para>

Perhaps the part from "If the old cluster is prior..." should be in a
new paragraph.

======
doc/src/sgml/logical-replication.sgml

2.
+   <para>
+    Setup the <link linkend="logical-replication-config-subscriber">
+    subscriber configurations</link> in the new subscriber.
+    <application>pg_upgrade</application> attempts to migrate subscription
+    dependencies which includes the subscription's table information present in
+    <link linkend="catalog-pg-subscription-rel">pg_subscription_rel</link>
+    system catalog and also the subscription's replication origin. This allows
+    logical replication on the new subscriber to continue from where the
+    old subscriber was up to. Migration of subscription dependencies is only
+    supported when the old cluster is version 17.0 or later. Subscription
+    dependencies on clusters before version 17.0 will silently be ignored.
+   </para>

Perhaps the part from "pg_upgrade attempts..." should be in a new paragraph.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Dear Vignesh,

Thanks for updating the patch! For now, v4 patch LGTM.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

On Mon, 29 Jan 2024 at 06:34, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh,
>
> Here are some review comments for patch v4.
>
> These are cosmetic only; otherwise v4 LGTM.
>
> ======
> doc/src/sgml/ref/pgupgrade.sgml
>
> 1.
>         Configure the servers for log shipping.  (You do not need to run
>         <function>pg_backup_start()</function> and
> <function>pg_backup_stop()</function>
>         or take a file system backup as the standbys are still synchronized
> -       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.
> +       with the primary.)  If the old cluster is prior to 17.0, then no slots
> +       on the primary are copied to the new standby, so all the slots must be
> +       recreated manually. If the old cluster is 17.0 or later, then 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.
>        </para>
>
> Perhaps the part from "If the old cluster is prior..." should be in a
> new paragraph.

Modified

> ======
> doc/src/sgml/logical-replication.sgml
>
> 2.
> +   <para>
> +    Setup the <link linkend="logical-replication-config-subscriber">
> +    subscriber configurations</link> in the new subscriber.
> +    <application>pg_upgrade</application> attempts to migrate subscription
> +    dependencies which includes the subscription's table information present in
> +    <link linkend="catalog-pg-subscription-rel">pg_subscription_rel</link>
> +    system catalog and also the subscription's replication origin. This allows
> +    logical replication on the new subscriber to continue from where the
> +    old subscriber was up to. Migration of subscription dependencies is only
> +    supported when the old cluster is version 17.0 or later. Subscription
> +    dependencies on clusters before version 17.0 will silently be ignored.
> +   </para>
>
> Perhaps the part from "pg_upgrade attempts..." should be in a new paragraph.

Modified

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

Regards,
Vignesh

On Mon, Jan 29, 2024 at 10:10 AM vignesh C <vignesh21@gmail.com> wrote:
>
> Thanks for the comments, the attached v5 version patch has the changes
> for the same.

Thanks for working on this. Here are some comments on the v5 patch:

1.
+  <para>
+   Migration of logical replication clusters is possible only when all the
+   members of the old logical replication clusters are version 17.0 or later.

Perhaps define what logical replication cluster is either in glossary
or within a parenthesis next to the first use in the docs? This will
help developers understand it better and will not confuse them with
postgres cluster. I see it being used for the first time in code
comments 9a17be1e2, but this patch uses it for the first time in the
docs.

2.
+   Before reading this section, refer <xref linkend="pgupgrade"/> page for
+   more details about pg_upgrade.
+  </para>

This looks extraneous, we can just link to pg_upgrade on the first use
of pg_upgrade, change the following

+   <para>
+    <application>pg_upgrade</application> attempts to migrate logical
+    slots. This helps avoid the need for manually defining the same

to

+   <para>
+    <xref linkend="pgupgrade"/> attempts to migrate logical
+    slots. This helps avoid the need for manually defining the same

3.
+     transactional, the user is advised to take backups. Backups can be taken
+     as described in <xref linkend="backup-base-backup"/>.
+    </para>

How about simplifying the above to "the user is advised to take
backups as described in <xref linkend="backup-base-backup"/>" instead
of two statements?

4.
  subscription is temporarily disabled, by executing
+    <link linkend="sql-altersubscription"><command>ALTER SUBSCRIPTION
... DISABLE</command></link>.
+    Re-enable the subscription after the upgrade.
+   </para>

Is it to avoid repeated failures of logical replication apply workers
on the subscribers? Isn't it good to say why subscription needs to be
disabled?

5.
+   <para>
+    There are some prerequisites for <application>pg_upgrade</application> to
+    be able to upgrade the logical slots. If these are not met an error
+    will be reported.
+   </para>

I think it's better to be "Following are prerequisites for
<application>pg_upgrade</application> to.."?

6.
+    <listitem>
+     <para>
+      The old cluster has replicated all the transactions and logical decoding
+      messages to subscribers.
+     </para>

I think it's better to be "The old cluster must have replicated all
the transactions and ...."?

7.
+     <para>
+      The new cluster must not have permanent logical slots, i.e.,
+      there must be no slots where
+      <link linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>temporary</structfield>
+      is <literal>false</literal>.

I think we better specify a full SQL query as opposed to just
specifying one output column and the view name.

<para>
 The new cluster must not have permanent logical slots, i.e., a query like:
<programlisting>
SELECT count(*) FROM pg_replication_slots WHERE slot_type = 'logical'
AND temporary IS false;
</programlisting>
    must return 0.
   </para>

8.
+       If the old cluster is prior to 17.0, then no slots on the primary are
+       copied to the new standby, so all the slots must be recreated manually.
+       If the old cluster is 17.0 or later, then only logical slots on the

I think it's better to say "version 17.0" instead of just "17.0".

9.
+       primary are copied to the new standby, but other slots on the
old standby

"but other slots on the old standby" - is it slots on the old standby
or old cluster?

I think it's the other way around: the old cluster needs to be
replaced with the old standby in the newly added paragraph.

10.
Change
+       primary are copied to the new standby, but other slots on the
old standby
+       are not copied so must be recreated manually.

to

+       primary are copied to the new standby, but other slots on the
old standby
+       are not copied, so must be recreated manually.

11.
+   <note>
+    <para>
+     The logical replication restrictions apply to logical replication cluster
+     upgrades also. See <xref linkend="logical-replication-restrictions"/> for
+     the details of logical replication restrictions.
+    </para>

How about just say "See <xref
linkend="logical-replication-restrictions"/> for details." instead of
using logical replication restrictions more than once in the same
para?

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

How about just say "See <xref linkend="prepare-publisher-upgrades"/>
for details." instead of using publisher upgrade prerequisites more
than once in the same para?

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

How about just say "See <xref linkend="prepare-subscriber-upgrades"/>
for details." instead of using subscriber upgrade prerequisites more
than once in the same para?

14.
+     Upgrading logical replication cluster requires multiple steps to be
+     performed on various nodes. Because not all operations are

Per comment #1, defining logical replication clusters and nodes helps
clearly distinguish. For instance, one can get confused with the
various terms in hand - postgres cluster, logical replication cluster,
node etc.

15.
+      two subscriptions <literal>sub1_node1_node2</literal> and
+      <literal>sub2_node1_node2</literal> which are subscribing the changes
+      from <literal>node1</literal>.

Why confluse with subsription names by including node1 and node2 in
it? We are not creating subscriptions from node1 to node2, are we? I'd
recommend using simplified names like mysub1, mysub2 like elsewhere in
the documentation.

16.
+      Let's say publisher is in <literal>node1</literal> and subscriber is
+      in <literal>node2</literal>.

How about saying "publisher is in a database cluster named
<literal>node1</literal> and subscriber is in database cluster named
<literal>node2</literal>"? I think using this terminology helps.

17.
+     refer to <xref linkend="logical-replication-upgrade"/> for details.
+    </para>
+   </note>

IMHO, it could have been better if steps to upgrade the logical
replication cluster is specified in pgupgrade.sgml as opposed to
logical-replication.sgml. Because, upgrading logical replication
cluster is a sub-section for pg_upgrade.

18.
+   <para>
+    The steps to upgrade the following logical replication clusters are
+    detailed below:
+    <itemizedlist>
+     <listitem>
+      <para>
+       Follow the steps specified in

I think we can talk about what advantages upgrading logical
replication clusters brings in. We can say that the pg_upgrade makes
it possible 1) to re-use the logical replication slots post-upgrade,
2) to re-use the subscribers i.e. now it's not required to re-create
all the logical subscribers after the upgrade, so no initial table
sync, no creation of new clusters for subscribers etc.

19. I think we can talk about the possible gotchas i.e. the things
that can go wrong while performing any of the prescribed steps. What
happens if the slot the pg_upgrade interrupts after it upgraded a few
of the replication slots or if some of the prerequisites are not met
etc.?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, 29 Jan 2024 at 16:01, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Jan 29, 2024 at 10:10 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments, the attached v5 version patch has the changes
> > for the same.
>
> Thanks for working on this. Here are some comments on the v5 patch:
>
> 1.
> +  <para>
> +   Migration of logical replication clusters is possible only when all the
> +   members of the old logical replication clusters are version 17.0 or later.
>
> Perhaps define what logical replication cluster is either in glossary
> or within a parenthesis next to the first use in the docs? This will
> help developers understand it better and will not confuse them with
> postgres cluster. I see it being used for the first time in code
> comments 9a17be1e2, but this patch uses it for the first time in the
> docs.

I have added it in glossary.

> 2.
> +   Before reading this section, refer <xref linkend="pgupgrade"/> page for
> +   more details about pg_upgrade.
> +  </para>
>
> This looks extraneous, we can just link to pg_upgrade on the first use
> of pg_upgrade, change the following
>
> +   <para>
> +    <application>pg_upgrade</application> attempts to migrate logical
> +    slots. This helps avoid the need for manually defining the same
>
> to
>
> +   <para>
> +    <xref linkend="pgupgrade"/> attempts to migrate logical
> +    slots. This helps avoid the need for manually defining the same

Modified

> 3.
> +     transactional, the user is advised to take backups. Backups can be taken
> +     as described in <xref linkend="backup-base-backup"/>.
> +    </para>
>
> How about simplifying the above to "the user is advised to take
> backups as described in <xref linkend="backup-base-backup"/>" instead
> of two statements?

Modified

> 4.
>   subscription is temporarily disabled, by executing
> +    <link linkend="sql-altersubscription"><command>ALTER SUBSCRIPTION
> ... DISABLE</command></link>.
> +    Re-enable the subscription after the upgrade.
> +   </para>
>
> Is it to avoid repeated failures of logical replication apply workers
> on the subscribers? Isn't it good to say why subscription needs to be
> disabled?

Added it

> 5.
> +   <para>
> +    There are some prerequisites for <application>pg_upgrade</application> to
> +    be able to upgrade the logical slots. If these are not met an error
> +    will be reported.
> +   </para>
>
> I think it's better to be "Following are prerequisites for
> <application>pg_upgrade</application> to.."?

Modified

> 6.
> +    <listitem>
> +     <para>
> +      The old cluster has replicated all the transactions and logical decoding
> +      messages to subscribers.
> +     </para>
>
> I think it's better to be "The old cluster must have replicated all
> the transactions and ...."?

Modified

> 7.
> +     <para>
> +      The new cluster must not have permanent logical slots, i.e.,
> +      there must be no slots where
> +      <link linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>temporary</structfield>
> +      is <literal>false</literal>.
>
> I think we better specify a full SQL query as opposed to just
> specifying one output column and the view name.
>
> <para>
>  The new cluster must not have permanent logical slots, i.e., a query like:
> <programlisting>
> SELECT count(*) FROM pg_replication_slots WHERE slot_type = 'logical'
> AND temporary IS false;
> </programlisting>
>     must return 0.
>    </para>

Modified

> 8.
> +       If the old cluster is prior to 17.0, then no slots on the primary are
> +       copied to the new standby, so all the slots must be recreated manually.
> +       If the old cluster is 17.0 or later, then only logical slots on the
>
> I think it's better to say "version 17.0" instead of just "17.0".

Modified

> 9.
> +       primary are copied to the new standby, but other slots on the
> old standby
>
> "but other slots on the old standby" - is it slots on the old standby
> or old cluster?
>
> I think it's the other way around: the old cluster needs to be
> replaced with the old standby in the newly added paragraph.

Modified it to old primary as we upgrade primary and do a rsync

> 10.
> Change
> +       primary are copied to the new standby, but other slots on the
> old standby
> +       are not copied so must be recreated manually.
>
> to
>
> +       primary are copied to the new standby, but other slots on the
> old standby
> +       are not copied, so must be recreated manually.

Modified

> 11.
> +   <note>
> +    <para>
> +     The logical replication restrictions apply to logical replication cluster
> +     upgrades also. See <xref linkend="logical-replication-restrictions"/> for
> +     the details of logical replication restrictions.
> +    </para>
>
> How about just say "See <xref
> linkend="logical-replication-restrictions"/> for details." instead of
> using logical replication restrictions more than once in the same
> para?

Modified

> 12.
> +    <para>
> +     The prerequisites of publisher upgrade apply to logical replication
> +     cluster upgrades also. See <xref linkend="prepare-publisher-upgrades"/>
> +     for the details of publisher upgrade prerequisites.
>
> How about just say "See <xref linkend="prepare-publisher-upgrades"/>
> for details." instead of using publisher upgrade prerequisites more
> than once in the same para?

Modified

> 13.
> +    <para>
> +     The prerequisites of subscriber upgrade apply to logical replication
> +     cluster upgrades also. See <xref linkend="prepare-subscriber-upgrades"/>
> +     for the details of subscriber upgrade prerequisites.
> +    </para>
>
> How about just say "See <xref linkend="prepare-subscriber-upgrades"/>
> for details." instead of using subscriber upgrade prerequisites more
> than once in the same para?

Modified

> 14.
> +     Upgrading logical replication cluster requires multiple steps to be
> +     performed on various nodes. Because not all operations are
>
> Per comment #1, defining logical replication clusters and nodes helps
> clearly distinguish. For instance, one can get confused with the
> various terms in hand - postgres cluster, logical replication cluster,
> node etc.

I have added "logical replication clusters". I felt no need to add
node as it is not a new terminology. It is already being used in many
places like in [1], [2] & [3]

> 15.
> +      two subscriptions <literal>sub1_node1_node2</literal> and
> +      <literal>sub2_node1_node2</literal> which are subscribing the changes
> +      from <literal>node1</literal>.
>
> Why confluse with subsription names by including node1 and node2 in
> it? We are not creating subscriptions from node1 to node2, are we? I'd
> recommend using simplified names like mysub1, mysub2 like elsewhere in
> the documentation.

I have used the name sub1_node1_node to indicate it is subscribing
changes from node1 to node2. I felt this is self explainatory names.

> 16.
> +      Let's say publisher is in <literal>node1</literal> and subscriber is
> +      in <literal>node2</literal>.
>
> How about saying "publisher is in a database cluster named
> <literal>node1</literal> and subscriber is in database cluster named
> <literal>node2</literal>"? I think using this terminology helps.

I felt existing is ok as similar is used in [2] & [3]

> 17.
> +     refer to <xref linkend="logical-replication-upgrade"/> for details.
> +    </para>
> +   </note>
>
> IMHO, it could have been better if steps to upgrade the logical
> replication cluster is specified in pgupgrade.sgml as opposed to
> logical-replication.sgml. Because, upgrading logical replication
> cluster is a sub-section for pg_upgrade.

As the content for logical replication is more, I felt it is better to
keep it here and also we have given a link to this in the pg_upgrade
page. I did not want the upgrade page to become bulky because of the
logical replication upgrade section.

> 18.
> +   <para>
> +    The steps to upgrade the following logical replication clusters are
> +    detailed below:
> +    <itemizedlist>
> +     <listitem>
> +      <para>
> +       Follow the steps specified in
>
> I think we can talk about what advantages upgrading logical
> replication clusters brings in. We can say that the pg_upgrade makes
> it possible 1) to re-use the logical replication slots post-upgrade,
> 2) to re-use the subscribers i.e. now it's not required to re-create
> all the logical subscribers after the upgrade, so no initial table
> sync, no creation of new clusters for subscribers etc.

I felt this is self explanatory, no need to mention about the
complexity involved in the manual steps involved. As the same is not
mentioned in case of streaming replication too at [4].

> 19. I think we can talk about the possible gotchas i.e. the things
> that can go wrong while performing any of the prescribed steps. What
> happens if the slot the pg_upgrade interrupts after it upgraded a few
> of the replication slots or if some of the prerequisites are not met
> etc.?

There is the note below when we run pg_upgrade:
"If pg_upgrade fails after this point, you must re-initdb the new
cluster before continuing."
I felt this is kind of self explanatory. Also the pre-requisite
mentions clearly about the configurations that must be set before
upgrade is run.  So I felt the existing information was enough.

Thanks for the comment, the attached v6 version patch has the changes
for the same.

[1] - https://www.postgresql.org/docs/devel/logical-replication.html
[2] - https://www.postgresql.org/docs/devel/logical-replication-publication.html
[3] - https://www.postgresql.org/docs/devel/logical-replication-subscription.html
[4] - https://www.postgresql.org/docs/devel/pgupgrade.html

Regards,
Vignesh

Dear Vignesh,

Thanks for updating the patch! Here are my comments for v6.

01.
```
+   <glossterm>Logical replication cluster</glossterm>
+   <glossdef>
+    <para>
+     A set of publisher and subscriber instance with publisher instance
+     replicating changes to the subscriber instance.
+    </para>
+   </glossdef>
```

Should we say 1:N relationship is allowed?

02.
```
@@ -70,6 +70,7 @@ PostgreSQL documentation
    pg_upgrade supports upgrades from 9.2.X and later to the current
    major release of <productname>PostgreSQL</productname>, including snapshot and beta releases.
   </para>
+
  </refsect1>
```

Unnecessary blank.

03.
```
   <para>
-   These are the steps to perform an upgrade
-   with <application>pg_upgrade</application>:
+   Below are the steps to perform an upgrade
+   with <application>pg_upgrade</application>.
   </para>
```

I'm not sure it should be included in this patch.

04.
```
+       If the old primary is prior to version 17.0, then no slots on the primary
+       are copied to the new standby, so all the slots on the old standby must
+       be recreated manually.
```

I think that "all the slots on the old standby" must be created manually in any
cases. Therefore, the preposition ", so" seems not correct.

05.
```
If the old primary is version 17.0 or later, then
+       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.
```

How about replacing this paragraph to below?

```
All the slots on the old standby must be recreated manually. If the old primary
is version 17.0 or later, then only logical slots on the primary are copied to the
new standby.
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

On Wed, 31 Jan 2024 at 11:42, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> Thanks for updating the patch! Here are my comments for v6.
>
> 01.
> ```
> +   <glossterm>Logical replication cluster</glossterm>
> +   <glossdef>
> +    <para>
> +     A set of publisher and subscriber instance with publisher instance
> +     replicating changes to the subscriber instance.
> +    </para>
> +   </glossdef>
> ```
>
> Should we say 1:N relationship is allowed?

I felt this need not be mentioned here, just wanted to give an
indication wherever this terminology is used, it means a set of
publisher and subscriber instances. Detail information should be added
in the logical replication related pages

> 02.
> ```
> @@ -70,6 +70,7 @@ PostgreSQL documentation
>     pg_upgrade supports upgrades from 9.2.X and later to the current
>     major release of <productname>PostgreSQL</productname>, including snapshot and beta releases.
>    </para>
> +
>   </refsect1>
> ```
>
> Unnecessary blank.

Removed it.

> 03.
> ```
>    <para>
> -   These are the steps to perform an upgrade
> -   with <application>pg_upgrade</application>:
> +   Below are the steps to perform an upgrade
> +   with <application>pg_upgrade</application>.
>    </para>
> ```
>
> I'm not sure it should be included in this patch.

This is not required in this patch, removed it.

> 04.
> ```
> +       If the old primary is prior to version 17.0, then no slots on the primary
> +       are copied to the new standby, so all the slots on the old standby must
> +       be recreated manually.
> ```
>
> I think that "all the slots on the old standby" must be created manually in any
> cases. Therefore, the preposition ", so" seems not correct.

I felt that this change is not related to this patch. I'm removing
these changes from the patch. Let's handle rephrasing of the base code
change in a separate thread.

> 05.
> ```
> If the old primary is version 17.0 or later, then
> +       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.
> ```
>
> How about replacing this paragraph to below?
>
> ```
> All the slots on the old standby must be recreated manually. If the old primary
> is version 17.0 or later, then only logical slots on the primary are copied to the
> new standby.
> ```

I felt that this change is not related to this patch. I'm removing
these changes from the patch. Let's handle rephrasing of the base code
change in a separate thread.

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

Regards,
Vignesh

Here are some review comments for patch v7-0001.

======
doc/src/sgml/glossary.sgml

1.
+  <glossentry id="glossary-logical-replication-cluster">
+   <glossterm>Logical replication cluster</glossterm>
+   <glossdef>
+    <para>
+     A set of publisher and subscriber instance with publisher instance
+     replicating changes to the subscriber instance.
+    </para>
+   </glossdef>
+  </glossentry>

1a.
/instance with/instances with/

~~~

1b.
The description then made me want to look up the glossary definition
of a "publisher instance" and "subscriber instance", but then I was
quite surprised that even "Publisher" and "Subscriber" terms are not
described in the glossary. Should this patch add those, or should we
start another thread for adding them?

======
doc/src/sgml/logical-replication.sgml

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

Here is where "logical replication clusters" is mentioned. Shouldn't
this first reference be linked to that new to the glossary entry --
e.g. <glossterm linkend="...">logical replication clusters</glossterm>

~~~

3.
+   <para>
+    Following are the prerequisites for <application>pg_upgrade</application>
+    to be able to upgrade the logical slots. If these are not met an error
+    will be reported.
+   </para>

SUGGESTION
The following prerequisites are required for ...

~~~

4.
+     <para>
+      All slots on the old cluster must be usable, i.e., there are no slots
+      whose
+      <link
linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>conflict_reason</structfield>
+      is not <literal>NULL</literal>.
+     </para>

The double-negative is too tricky "no slots whose ... not NULL", needs
rewording. Maybe it is better to instead use an example as the next
bullet point does.

~~~

5.
+
+   <para>
+    Following are the prerequisites for
<application>pg_upgrade</application> to
+    be able to upgrade the subscriptions. If these are not met an error
+    will be reported.
+   </para>

SUGGESTION
The following prerequisites are required for ...

======
doc/src/sgml/ref/pgupgrade.sgml

6.
+   <note>
+    <para>
+     The steps to upgrade logical replication clusters are not covered here;
+     refer to <xref linkend="logical-replication-upgrade"/> for details.
+    </para>
+   </note>

Maybe here too there should be a link to the glossary term "logical
replication clusters".

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, 1 Feb 2024 at 14:50, vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, 31 Jan 2024 at 11:42, Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Vignesh,
> >
> > Thanks for updating the patch! Here are my comments for v6.
> >
> > 01.
> > ```
> > +   <glossterm>Logical replication cluster</glossterm>
> > +   <glossdef>
> > +    <para>
> > +     A set of publisher and subscriber instance with publisher instance
> > +     replicating changes to the subscriber instance.
> > +    </para>
> > +   </glossdef>
> > ```
> >
> > Should we say 1:N relationship is allowed?
>
> I felt this need not be mentioned here, just wanted to give an
> indication wherever this terminology is used, it means a set of
> publisher and subscriber instances. Detail information should be added
> in the logical replication related pages
>
> > 02.
> > ```
> > @@ -70,6 +70,7 @@ PostgreSQL documentation
> >     pg_upgrade supports upgrades from 9.2.X and later to the current
> >     major release of <productname>PostgreSQL</productname>, including snapshot and beta releases.
> >    </para>
> > +
> >   </refsect1>
> > ```
> >
> > Unnecessary blank.
>
> Removed it.
>
> > 03.
> > ```
> >    <para>
> > -   These are the steps to perform an upgrade
> > -   with <application>pg_upgrade</application>:
> > +   Below are the steps to perform an upgrade
> > +   with <application>pg_upgrade</application>.
> >    </para>
> > ```
> >
> > I'm not sure it should be included in this patch.
>
> This is not required in this patch, removed it.
>
> > 04.
> > ```
> > +       If the old primary is prior to version 17.0, then no slots on the primary
> > +       are copied to the new standby, so all the slots on the old standby must
> > +       be recreated manually.
> > ```
> >
> > I think that "all the slots on the old standby" must be created manually in any
> > cases. Therefore, the preposition ", so" seems not correct.
>
> I felt that this change is not related to this patch. I'm removing
> these changes from the patch. Let's handle rephrasing of the base code
> change in a separate thread.
>
> > 05.
> > ```
> > If the old primary is version 17.0 or later, then
> > +       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.
> > ```
> >
> > How about replacing this paragraph to below?
> >
> > ```
> > All the slots on the old standby must be recreated manually. If the old primary
> > is version 17.0 or later, then only logical slots on the primary are copied to the
> > new standby.
> > ```
>
> I felt that this change is not related to this patch. I'm removing
> these changes from the patch. Let's handle rephrasing of the base code
> change in a separate thread.

A new thread is started for the same and a patch is attached at [1].
Let's discuss this change at the new thread.
[1] - https://www.postgresql.org/message-id/CAHv8RjJHCw0jpUo9PZxjcguzGt3j2W1_NH%3DQuREoN0nYiVdVeA%40mail.gmail.com

Regards,
Vignesh

On Fri, 9 Feb 2024 at 12:30, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for patch v7-0001.
>
> ======
> doc/src/sgml/glossary.sgml
>
> 1.
> +  <glossentry id="glossary-logical-replication-cluster">
> +   <glossterm>Logical replication cluster</glossterm>
> +   <glossdef>
> +    <para>
> +     A set of publisher and subscriber instance with publisher instance
> +     replicating changes to the subscriber instance.
> +    </para>
> +   </glossdef>
> +  </glossentry>
>
> 1a.
> /instance with/instances with/

Modified

> ~~~
>
> 1b.
> The description then made me want to look up the glossary definition
> of a "publisher instance" and "subscriber instance", but then I was
> quite surprised that even "Publisher" and "Subscriber" terms are not
> described in the glossary. Should this patch add those, or should we
> start another thread for adding them?

 I felt it is better to start a new thread for this

> ======
> doc/src/sgml/logical-replication.sgml
>
> 2.
> +  <para>
> +   Migration of logical replication clusters is possible only when all the
> +   members of the old logical replication clusters are version 17.0 or later.
> +  </para>
>
> Here is where "logical replication clusters" is mentioned. Shouldn't
> this first reference be linked to that new to the glossary entry --
> e.g. <glossterm linkend="...">logical replication clusters</glossterm>

Modified

> ~~~
>
> 3.
> +   <para>
> +    Following are the prerequisites for <application>pg_upgrade</application>
> +    to be able to upgrade the logical slots. If these are not met an error
> +    will be reported.
> +   </para>
>
> SUGGESTION
> The following prerequisites are required for ...
>
> ~~~
>
> 4.
> +     <para>
> +      All slots on the old cluster must be usable, i.e., there are no slots
> +      whose
> +      <link
linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>conflict_reason</structfield>
> +      is not <literal>NULL</literal>.
> +     </para>
>
> The double-negative is too tricky "no slots whose ... not NULL", needs
> rewording. Maybe it is better to instead use an example as the next
> bullet point does.

The other way is to mention "all slots should have conflic_reason is
NULL", but in this case i feel checking for records is not NULL is
better. So I have kept the wording the same and added an example to
avoid any confusion.

> ~~~
>
> 5.
> +
> +   <para>
> +    Following are the prerequisites for
> <application>pg_upgrade</application> to
> +    be able to upgrade the subscriptions. If these are not met an error
> +    will be reported.
> +   </para>
>
> SUGGESTION
> The following prerequisites are required for ...

Modified

> ======
> doc/src/sgml/ref/pgupgrade.sgml
>
> 6.
> +   <note>
> +    <para>
> +     The steps to upgrade logical replication clusters are not covered here;
> +     refer to <xref linkend="logical-replication-upgrade"/> for details.
> +    </para>
> +   </note>
>
> Maybe here too there should be a link to the glossary term "logical
> replication clusters".

Modified

Thanks for the comments, the attached v8 version patch has the changes
for the patch.

Regards,
Vignesh

On Mon, 12 Feb 2024 at 14:33, vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 9 Feb 2024 at 12:30, Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Here are some review comments for patch v7-0001.
> >
> > ======
> > doc/src/sgml/glossary.sgml
> >
> > 1.
> > +  <glossentry id="glossary-logical-replication-cluster">
> > +   <glossterm>Logical replication cluster</glossterm>
> > +   <glossdef>
> > +    <para>
> > +     A set of publisher and subscriber instance with publisher instance
> > +     replicating changes to the subscriber instance.
> > +    </para>
> > +   </glossdef>
> > +  </glossentry>
> >
> > 1a.
> > /instance with/instances with/
>
> Modified
>
> > ~~~
> >
> > 1b.
> > The description then made me want to look up the glossary definition
> > of a "publisher instance" and "subscriber instance", but then I was
> > quite surprised that even "Publisher" and "Subscriber" terms are not
> > described in the glossary. Should this patch add those, or should we
> > start another thread for adding them?
>
>  I felt it is better to start a new thread for this

A new patch has been posted at [1] to address this.
[1] -
https://www.postgresql.org/message-id/CANhcyEXa%3D%2BshzbdS2iW9%3DY%3D_Eh7aRWZbQKJjDHVYiCmuiE1Okw%40mail.gmail.com

Regards,
Vignesh

Here are some minor comments for patch v8-0001.

======
doc/src/sgml/glossary.sgml

1.
+   <glossdef>
+    <para>
+     A set of publisher and subscriber instances with publisher instance
+     replicating changes to the subscriber instance.
+    </para>
+   </glossdef>

/with publisher instance/with the publisher instance/

~~~

2.
There are 2 SQL fragments but they are wrapped differently (see
below). e.g. it is not clear why is the 2nd fragment wrapped since it
is shorter than the 1st. OTOH, maybe you want the 1st fragment to
wrap. Either way, consistency wrapping would be better.


postgres=# SELECT slot_name FROM pg_replication_slots WHERE slot_type
= 'logical' AND conflict_reason IS NOT NULL;
 slot_name
-----------
(0 rows)

versus

SELECT count(*) FROM pg_replication_slots WHERE slot_type = 'logical'
AND temporary IS false;

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, 22 Feb 2024 at 09:35, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some minor comments for patch v8-0001.
>
> ======
> doc/src/sgml/glossary.sgml
>
> 1.
> +   <glossdef>
> +    <para>
> +     A set of publisher and subscriber instances with publisher instance
> +     replicating changes to the subscriber instance.
> +    </para>
> +   </glossdef>
>
> /with publisher instance/with the publisher instance/

Modified

> ~~~
>
> 2.
> There are 2 SQL fragments but they are wrapped differently (see
> below). e.g. it is not clear why is the 2nd fragment wrapped since it
> is shorter than the 1st. OTOH, maybe you want the 1st fragment to
> wrap. Either way, consistency wrapping would be better.

Modified

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

I have added a commitfest entry for this:
https://commitfest.postgresql.org/47/4848/

Regards,
Vignesh

Hi Vignesh, I have no further comments. Patch v9 LGTM.

==========
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, 23 Feb 2024 at 04:58, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh, I have no further comments. Patch v9 LGTM.

The v9 version patch was not applying on top of HEAD because of few
commits, the updated v10 version patch is rebased on top of HEAD.

Regards,
Vignesh