On Wed, Feb 8, 2023 at 8:03 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
...
> > ======
> >
> > src/backend/replication/logical/worker.c
> >
> > 2. maybe_apply_delay
> >
> > + if (wal_receiver_status_interval > 0 &&
> > + diffms > wal_receiver_status_interval * 1000L)
> > + {
> > + WaitLatch(MyLatch,
> > + WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
> > + wal_receiver_status_interval * 1000L,
> > + WAIT_EVENT_RECOVERY_APPLY_DELAY);
> > + send_feedback(last_received, true, false, true);
> > + }
> >
> > I felt that introducing another variable like:
> >
> > long statusinterval_ms = wal_receiver_status_interval * 1000L;
> >
> > would help here by doing 2 things:
> > 1) The condition would be easier to read because the ms units would be the same
> > 2) Won't need * 1000L repeated in two places.
> >
> > Only, do take care to assign this variable in the right place in this
> > loop in case the configuration is changed.
>
> Fixed. Calculations are done on two lines - first one is the entrance of the loop,
> and second one is the after SIGHUP is detected.
>
TBH, I expected you would write this as just a *single* variable
assignment before the condition like below:
SUGGESTION (tweaked comment and put single assignment before condition)
/*
* Call send_feedback() to prevent the publisher from exiting by
* timeout during the delay, when the status interval is greater than
* zero.
*/
status_interval_ms = wal_receiver_status_interval * 1000L;
if (status_interval_ms > 0 && diffms > status_interval_ms)
{
...
~
I understand in theory, your code is more efficient, but in practice,
I think the overhead of a single variable assignment every loop
iteration (which is doing WaitLatch anyway) is of insignificant
concern, whereas having one assignment is simpler than having two IMO.
But, if you want to keep it the way you have then that is OK.
Otherwise, this patch v32 LGTM.
------
Kind Regards,
Peter Smith.
Fujitsu Australia.