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
