Re: Documentation to upgrade logical replication cluster

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