Hi,
On Monday, November 14, 2022 7:15 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > On Wed, Nov 9, 2022 at 12:11 PM Kyotaro Horiguchi > <horikyota....@gmail.com> wrote: > > > > At Wed, 10 Aug 2022 17:33:00 -0300, "Euler Taveira" > > <eu...@eulerto.com> wrote in > > > On Wed, Aug 10, 2022, at 9:39 AM, osumi.takami...@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
