Re: Failed transaction statistics to measure the logical replication progress

On Thu, Feb 17, 2022 at 3:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jan 4, 2022 at 5:22 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> >
> > On Friday, December 31, 2021 10:12 AM Tang, Haiying/唐 海英 <tanghy.fnst@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.