Thread: Documentation: warn about two_phase when altering a subscription
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
Attachment
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
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
Attachment
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
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.
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 thepatch itself, I think so far it's good but would make two modifications. I like how the patch was worded originally whenreferring to the subscription, stating these parameters were 'in' the subscription rather than 'by' it. So I'd proposechanging > 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
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 sofar 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
> 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.
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.
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 thepatch itself, I think so far it's good but would make two modifications. I like how the patch was worded originally whenreferring to the subscription, stating these parameters were 'in' the subscription rather than 'by' it. So I'd proposechanging > > > 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.