Hi,
On Monday, November 14, 2022 7:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Nov 9, 2022 at 12:11 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > At Wed, 10 Aug 2022 17:33:00 -0300, "Euler Taveira"
> > <euler@eulerto.com> wrote in
> > > On Wed, Aug 10, 2022, at 9:39 AM, osumi.takamichi@fujitsu.com wrote:
> > > > Minor review comments for v6.
> > > Thanks for your review. I'm attaching v7.
> >
> > Using interval is not standard as this kind of parameters but it seems
> > convenient. On the other hand, it's not great that the unit month
> > introduces some subtle ambiguity. This patch translates a month to 30
> > days but I'm not sure it's the right thing to do. Perhaps we shouldn't
> > allow the units upper than days.
> >
>
> Agreed. Isn't the same thing already apply to recovery_min_apply_delay for
> which the maximum unit seems to be in days? If so, there is no reason to do
> something different here?
The corresponding one of physical replication had the
upper limit of INT_MAX(like it means 24 days is OK, but 25 days isn't).
I added this test in the patch posted in [1].
>
> > apply_delay() chokes the message-receiving path so that a not-so-long
> > delay can cause a replication timeout to fire. I think we should
> > process walsender pings even while delaying. Needing to make
> > replication timeout longer than apply delay is not great, I think.
> >
>
> Again, I think for this case also the behavior should be similar to how we handle
> recovery_min_apply_delay.
Yes, I agree with you.
This feature makes it easier to trigger the publisher's timeout,
which can't be observed in the physical replication.
I'll do the investigation and modify this point in a subsequent version.
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373775ECC6972289AF8CB30ED0F9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
