Hi,
On Wednesday, February 1, 2023 6:41 PM Shi, Yu/侍 雨 <shiy.fnst@fujitsu.com> wrote:
> On Mon, Jan 30, 2023 6:05 PM Takamichi Osumi (Fujitsu)
> <osumi.takamichi@fujitsu.com> wrote:
> >
> > On Saturday, January 28, 2023 1:28 PM I wrote:
> > > Attached the updated patch v24.
> > Hi,
> >
> >
> > I've conducted the rebase affected by the commit(1e8b61735c) by
> > renaming the GUC to logical_replication_mode accordingly, because it's
> > utilized in the TAP test of this time-delayed LR feature.
> > There is no other change for this version.
> >
> > Kindly have a look at the attached v25.
> >
>
> Thanks for your patch. Here are some comments.
Thank you for your review !
> 2.
> +# Make sure the apply worker knows to wait for more than 500ms
> +check_apply_delay_log($node_subscriber, $offset, "0.5");
>
> I think the last parameter should be 500.
Good catch ! Fixed.
> Besides, I am not sure it's a stable test to check the log. Is it possible that there's
> no such log on a slow machine? I modified the code to sleep 1s at the beginning
> of apply_dispatch(), then the new added test failed because the server log
> cannot match.
To get the log by itself is necessary to ensure
that the delay is conducted by the apply worker, because we emit the diffms
only if it's bigger than 0 in maybe_apply_delay(). If we omit the step,
we are not sure the delay is caused by other reasons or the time-delayed feature.
As you mentioned, it's possible that no log is emitted on slow machine. Then,
the idea to make the test safer for such machines should be to make the delayed time longer.
But we shortened the delay time to 1 second to mitigate the long test execution time of this TAP test.
So, I'm not sure if it's a good idea to make it longer again.
Please have a look at the latest v26 in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB83730A45925B9680C40D92AFEDD69%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
