Horton

Thread

Commits

  1. Doc: Warn about two_phase when altering a subscription's slot name.

  1. Documentation: warn about two_phase when altering a subscription

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-01-30T15:41:32Z

    Hi hackers,

776621a5e4 added a "warning" in the documentation to alter a subscription (to
ensure the slot's failover property matches the subscription's one).

The same remark could be done for the two_phase option. This patch is an attempt
to do so.

Looking forward to your feedback,

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  2. Re: Documentation: warn about two_phase when altering a subscription

    Peter Smith <smithpb2250@gmail.com> — 2024-01-31T02:47:16Z

    Hi, thanks for the patch. Here are some review comments for v1

======

(below is not showing the links and other sgml rendering -- all those LGTM)

BEFORE
When altering the slot_name, the failover and two_phase properties
values of the named slot may differ from their counterparts failover
and two_phase parameters specified in the subscription. When creating
the slot, ensure the slot failover and two_phase properties match
their counterparts parameters values of the subscription.

SUGGESTION
When altering the slot_name, the failover and two_phase property
values of the named slot may differ from the counterpart failover and
two_phase parameters specified by the subscription. When creating the
slot, ensure the slot properties failover and two_phase match their
counterpart parameters of the subscription.

~

BEFORE
Otherwise, the slot on the publisher may behave differently from what
subscription's failover and two_phase options say: for example, the
slot on the publisher could ...

SUGGESTION:
Otherwise, the slot on the publisher may behave differently from what
these subscription options say: for example, the slot on the publisher
could ...

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

  3. Re: Documentation: warn about two_phase when altering a subscription

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-01-31T05:55:39Z

    Hi,

On Wed, Jan 31, 2024 at 01:47:16PM +1100, Peter Smith wrote:
> Hi, thanks for the patch. Here are some review comments for v1

Thanks for the review!

> 
> ======
> 
> (below is not showing the links and other sgml rendering -- all those LGTM)
> 
> BEFORE
> When altering the slot_name, the failover and two_phase properties
> values of the named slot may differ from their counterparts failover
> and two_phase parameters specified in the subscription. When creating
> the slot, ensure the slot failover and two_phase properties match
> their counterparts parameters values of the subscription.
> 
> SUGGESTION
> When altering the slot_name, the failover and two_phase property
> values of the named slot may differ from the counterpart failover and
> two_phase parameters specified by the subscription. When creating the
> slot, ensure the slot properties failover and two_phase match their
> counterpart parameters of the subscription.
> 
> ~
> 
> BEFORE
> Otherwise, the slot on the publisher may behave differently from what
> subscription's failover and two_phase options say: for example, the
> slot on the publisher could ...
> 
> SUGGESTION:
> Otherwise, the slot on the publisher may behave differently from what
> these subscription options say: for example, the slot on the publisher
> could ...
> 

As a non native English speaker somehow I have to rely on you for those
suggestions ;-)

They make sense to me so applied both in v2 attached.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  4. Re: Documentation: warn about two_phase when altering a subscription

    Peter Smith <smithpb2250@gmail.com> — 2024-01-31T22:48:05Z

    On Wed, Jan 31, 2024 at 4:55 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
...
> As a non native English speaker somehow I have to rely on you for those
> suggestions ;-)
>
> They make sense to me so applied both in v2 attached.
>

Patch v2 looks OK to me, but probably there is still room for
improvement. Let's see what others think.

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

  5. Re: Documentation: warn about two_phase when altering a subscription

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

    On Wed, Jan 31, 2024 at 11:25 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> They make sense to me so applied both in v2 attached.
>

This patch looks good to me but I think it is better to commit this
after we push some more work on failover slots, especially the core
sync slot functionality because we are changing the existing wording
of the related work.

-- 
With Regards,
Amit Kapila.

  6. Re: Documentation: warn about two_phase when altering a subscription

    Tristen Raab <tristen.raab@highgo.ca> — 2024-02-23T23:50:17Z

    The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            tested, passed

Hello,

I've reviewed your patch, it applies correctly and the documentation builds without any errors. As for the content of the patch itself, I think so far it's good but would make two modifications. I like how the patch was worded originally when referring to the subscription, stating these parameters were 'in' the subscription rather than 'by' it. So I'd propose changing

> parameters specified by the subscription. When creating the slot, ensure

to 

> parameters specified in the subscription. When creating the slot, ensure

and secondly the section "ensure the slot properties failover and two_phase match their counterpart parameters of the subscription" sounds a bit clunky. So I'd also propose changing:

> the slot properties <literal>failover</literal> and <literal>two_phase</literal>
> match their counterpart parameters of the subscription.

to

> the slot properties <literal>failover</literal> and <literal>two_phase</literal>
> match their counterpart parameters in the subscription.

I feel this makes the description flow a bit better when reading. But other than that I think it's quite clear.

kind regards,

-----------------------
Tristen Raab
Highgo Software Canada
www.highgo.ca

  7. Re: Documentation: warn about two_phase when altering a subscription

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-02-26T09:14:07Z

    Hi,

On Fri, Feb 23, 2024 at 11:50:17PM +0000, Tristen Raab wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:       tested, passed
> Spec compliant:           not tested
> Documentation:            tested, passed
> 
> Hello,
> 
> I've reviewed your patch,

Thanks!

> it applies correctly and the documentation builds without any errors. As for the content of the patch itself, I think so far it's good but would make two modifications. I like how the patch was worded originally when referring to the subscription, stating these parameters were 'in' the subscription rather than 'by' it. So I'd propose changing
> 
> > parameters specified by the subscription. When creating the slot, ensure
> 
> to 
> 
> > parameters specified in the subscription. When creating the slot, ensure

As non native English speaker I don't have a strong opinion on that (though I
also preferred how it was worded in V1).

> 
> and secondly the section "ensure the slot properties failover and two_phase match their counterpart parameters of the subscription" sounds a bit clunky. So I'd also propose changing:
> 
> > the slot properties <literal>failover</literal> and <literal>two_phase</literal>
> > match their counterpart parameters of the subscription.
> 
> to
> 
> > the slot properties <literal>failover</literal> and <literal>two_phase</literal>
> > match their counterpart parameters in the subscription.

Same here, I don't have a strong opinion on that.

As the patch as it is now looks good to Amit (see [1]), I prefer to let him
decide which wording he pefers to push.

> I feel this makes the description flow a bit better when reading. But other than that I think it's quite clear.

Thanks!

[1]: https://www.postgresql.org/message-id/CAA4eK1KdZMtJfo%3DK%3DXWsQQu8OHutT_dwJV2D3zs4h9g5zdNz2A%40mail.gmail.com

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  8. Re: Documentation: warn about two_phase when altering a subscription

    x4mmm@yandex-team.ru — 2024-03-02T18:14:35Z

    
> On 26 Feb 2024, at 14:14, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> 
> As the patch as it is now looks good to Amit (see [1]), I prefer to let him
> decide which wording he pefers to push.

Added Amit to cc, just to be sure. Thanks!


Best regards, Andrey Borodin, learning how to be CFM.

  9. Re: Documentation: warn about two_phase when altering a subscription

    Amit Kapila <amit.kapila16@gmail.com> — 2024-03-03T04:03:19Z

    On Sat, Mar 2, 2024 at 11:44 PM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
> > On 26 Feb 2024, at 14:14, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> >
> > As the patch as it is now looks good to Amit (see [1]), I prefer to let him
> > decide which wording he pefers to push.
>
> Added Amit to cc, just to be sure. Thanks!
>

I'll look into it during this CF.

-- 
With Regards,
Amit Kapila.

  10. Re: Documentation: warn about two_phase when altering a subscription

    Amit Kapila <amit.kapila16@gmail.com> — 2024-03-11T04:15:27Z

    On Sat, Feb 24, 2024 at 5:21 AM Tristen Raab <tristen.raab@highgo.ca> wrote:
>
> I've reviewed your patch, it applies correctly and the documentation builds without any errors. As for the content of the patch itself, I think so far it's good but would make two modifications. I like how the patch was worded originally when referring to the subscription, stating these parameters were 'in' the subscription rather than 'by' it. So I'd propose changing
>
> > parameters specified by the subscription. When creating the slot, ensure
>
> to
>
> > parameters specified in the subscription. When creating the slot, ensure
>

This suggestion makes sense, so I updated the patch for this and committed.

-- 
With Regards,
Amit Kapila.