On Thu, Feb 17, 2022 at 3:13 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Tue, Jan 4, 2022 at 5:22 PM osumi.takami...@fujitsu.com
> <osumi.takami...@fujitsu.com> wrote:
> >
> > On Friday, December 31, 2021 10:12 AM Tang, Haiying/唐 海英
> > <tanghy.f...@fujitsu.com> wrote:
> > > 4)
> > > +void
> > > +pgstat_send_subworker_xact_stats(LogicalRepWorker *repWorker, bool
> > > +force) {
> > > + static TimestampTz last_report = 0;
> > > + PgStat_MsgSubWorkerXactEnd msg;
> > > +
> > > + if (!force)
> > > + {
> > > ...
> > > + if (!TimestampDifferenceExceeds(last_report, now,
> > > PGSTAT_STAT_INTERVAL))
> > > + return;
> > > + last_report = now;
> > > + }
> > > +
> > > ...
> > > + if (repWorker->commit_count == 0 && repWorker->abort_count ==
> > > 0)
> > > + return;
> > > ...
> > >
> > > I think it's better to check commit_count and abort_count first, then
> > > check if
> > > reach PGSTAT_STAT_INTERVAL.
> > > Otherwise if commit_count and abort_count are 0, it is possible that the
> > > value
> > > of last_report has been updated but it didn't send stats in fact. In this
> > > case,
> > > last_report is not the real time that send last message.
> > Yeah, agreed. This fix is right in terms of the variable name aspect.
> >
>
> Can't we use pgstat_report_stat() here? Basically, you can update xact
> completetion counters during apply, and then from
> pgstat_report_stat(), you can invoke a logical replication worker
> stats-related function.
>
If we can do this then we can save the logic this patch is trying to
introduce for PGSTAT_STAT_INTERVAL.
--
With Regards,
Amit Kapila.
