On Mon, 15 Jan 2024 at 14:39, Hayato Kuroda (Fujitsu) <kuroda.hay...@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
