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
