Обсуждение: src/bin/pg_upgrade/t/004_subscription.pl test comment fix
Hi, PSA a small fix for a misleading comment found in the pg_upgrade test code. ====== Kind Regards, Peter Smith. Fujitsu Australia
Вложения
On Wed, 31 Jan 2024 at 10:27, Peter Smith <smithpb2250@gmail.com> wrote: > > Hi, > > PSA a small fix for a misleading comment found in the pg_upgrade test code. Is the double # intentional here, as I did not see this style of commenting used elsewhere: +# # Upgraded regress_sub1 should still have enabled and failover as true. +# # Upgraded regress_sub2 should still have enabled and failover as false. Regards, Vignesh
On Wed, Jan 31, 2024 at 4:51 PM vignesh C <vignesh21@gmail.com> wrote: > > On Wed, 31 Jan 2024 at 10:27, Peter Smith <smithpb2250@gmail.com> wrote: > > > > Hi, > > > > PSA a small fix for a misleading comment found in the pg_upgrade test code. > > Is the double # intentional here, as I did not see this style of > commenting used elsewhere: > +# # Upgraded regress_sub1 should still have enabled and failover as true. > +# # Upgraded regress_sub2 should still have enabled and failover as false. > Unintentional caused by copy/paste. Thanks for reporting. I will post a fixed patch tomorrow. ====== Kind Regards, Peter Smith. Fujitsu Australia
How about rewording it more extensively? It doesn't read great IMO. I would use something like # In the upgraded instance, the running status and failover option of the # subscription with the failover option should have been preserved; the other # should not. # So regress_sub1 should still have subenabled,subfailover set to true, # while regress_sub2 should have both set to false. I think the symmetry between the two lines confuses more than helps. It's not a huge thing but since we're editing anyway, why not? -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "The saddest aspect of life right now is that science gathers knowledge faster than society gathers wisdom." (Isaac Asimov)
On Wed, Jan 31, 2024 at 7:48 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > How about rewording it more extensively? It doesn't read great IMO. > I would use something like > > # In the upgraded instance, the running status and failover option of the > # subscription with the failover option should have been preserved; the other > # should not. > # So regress_sub1 should still have subenabled,subfailover set to true, > # while regress_sub2 should have both set to false. > IIUC this suggested comment is implying that the running status is *only* preserved when the failover option is true. But AFAIK that is not correct. e.g. I hacked the test to keep subscription regress_sub2 as ENABLED but the result after the upgrade was subenabled/subfailover = t/f, not f/f. > I think the symmetry between the two lines confuses more than helps. > It's not a huge thing but since we're editing anyway, why not? > OK. Now using your suggested 2nd sentence: +# The subscription's running status and failover option should be preserved +# in the upgraded instance. So regress_sub1 should still have subenabled,subfailover +# set to true, while regress_sub2 should have both set to false. ~ I also tweaked some other nearby comments/messages which did not mention the 'failover' preservation. PSA v2. ====== Kind Regards, Peter Smith. Fujitsu Australia
Вложения
On Thu, Feb 1, 2024 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote: > > OK. Now using your suggested 2nd sentence: > > +# The subscription's running status and failover option should be preserved > +# in the upgraded instance. So regress_sub1 should still have > subenabled,subfailover > +# set to true, while regress_sub2 should have both set to false. > > ~ > > I also tweaked some other nearby comments/messages which did not > mention the 'failover' preservation. > Looks mostly good to me. One minor nitpick: * along with retaining the replication origin's remote lsn -# and subscription's running status. +# and subscription's running status and failover option. I don't think we need to use 'and' twice in the above sentence. We should use ',' between different properties. I can change this on Monday and push it unless you think otherwise. -- With Regards, Amit Kapila.
On Sat, Feb 3, 2024 at 5:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Thu, Feb 1, 2024 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > OK. Now using your suggested 2nd sentence: > > > > +# The subscription's running status and failover option should be preserved > > +# in the upgraded instance. So regress_sub1 should still have > > subenabled,subfailover > > +# set to true, while regress_sub2 should have both set to false. > > > > ~ > > > > I also tweaked some other nearby comments/messages which did not > > mention the 'failover' preservation. > > > > Looks mostly good to me. One minor nitpick: > * > along with retaining the replication origin's remote lsn > -# and subscription's running status. > +# and subscription's running status and failover option. > > I don't think we need to use 'and' twice in the above sentence. We > should use ',' between different properties. I can change this on > Monday and push it unless you think otherwise. > +1 ====== Kind Regards, Peter Smith. Fujitsu Australia
On Mon, Feb 5, 2024 at 2:42 AM Peter Smith <smithpb2250@gmail.com> wrote: > > On Sat, Feb 3, 2024 at 5:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Thu, Feb 1, 2024 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > > > OK. Now using your suggested 2nd sentence: > > > > > > +# The subscription's running status and failover option should be preserved > > > +# in the upgraded instance. So regress_sub1 should still have > > > subenabled,subfailover > > > +# set to true, while regress_sub2 should have both set to false. > > > > > > ~ > > > > > > I also tweaked some other nearby comments/messages which did not > > > mention the 'failover' preservation. > > > > > > > Looks mostly good to me. One minor nitpick: > > * > > along with retaining the replication origin's remote lsn > > -# and subscription's running status. > > +# and subscription's running status and failover option. > > > > I don't think we need to use 'and' twice in the above sentence. We > > should use ',' between different properties. I can change this on > > Monday and push it unless you think otherwise. > > > > +1 > Pushed! -- With Regards, Amit Kapila.