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