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
