Re: Time delayed LR (WAS Re: logical replication restrictions) - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Time delayed LR (WAS Re: logical replication restrictions)
Date
Msg-id CAA4eK1Jfga+rJ5G1fEBYSEt6E3sTLMUbvKqkovMYAqqoBZKbzg@mail.gmail.com
Whole thread Raw
In response to Re: Time delayed LR (WAS Re: logical replication restrictions)  (Peter Smith <smithpb2250@gmail.com>)
Responses RE: Time delayed LR (WAS Re: logical replication restrictions)
Re: Time delayed LR (WAS Re: logical replication restrictions)
List pgsql-hackers
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.



pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: [PATCH] Add pretty-printed XML output option
Next
From: Jim Jones
Date:
Subject: Re: [PATCH] Add pretty-printed XML output option