Horton

Thread

Commits

  1. Improve the comments in 004_subscription.pl.

  1. src/bin/pg_upgrade/t/004_subscription.pl test comment fix

    Peter Smith <smithpb2250@gmail.com> — 2024-01-31T04:56:50Z

    Hi,

PSA a small fix for a misleading comment found in the pg_upgrade test code.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

  2. Re: src/bin/pg_upgrade/t/004_subscription.pl test comment fix

    vignesh C <vignesh21@gmail.com> — 2024-01-31T05:51:03Z

    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

  3. Re: src/bin/pg_upgrade/t/004_subscription.pl test comment fix

    Peter Smith <smithpb2250@gmail.com> — 2024-01-31T06:18:51Z

    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

  4. Re: src/bin/pg_upgrade/t/004_subscription.pl test comment fix

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-01-31T08:48:00Z

    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)

  5. Re: src/bin/pg_upgrade/t/004_subscription.pl test comment fix

    Peter Smith <smithpb2250@gmail.com> — 2024-02-01T00:27:43Z

    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

  6. Re: src/bin/pg_upgrade/t/004_subscription.pl test comment fix

    Amit Kapila <amit.kapila16@gmail.com> — 2024-02-03T06:28:24Z

    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.

  7. Re: src/bin/pg_upgrade/t/004_subscription.pl test comment fix

    Peter Smith <smithpb2250@gmail.com> — 2024-02-04T21:12:18Z

    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

  8. Re: src/bin/pg_upgrade/t/004_subscription.pl test comment fix

    Amit Kapila <amit.kapila16@gmail.com> — 2024-02-05T05:05:21Z

    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.