On Thu, Feb 9, 2023 at 12:17 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.
>
Yeah, that sounds better to me as well.
--
With Regards,
Amit Kapila.