Hi,
On Thursday, February 9, 2023 4:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 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.
OK, fixed.
The comment adjustment suggested by Peter-san above
was also included in this v33.
Please have a look at the attached patch.
Best Regards,
Takamichi Osumi