Re: Design of pg_stat_subscription_workers vs pgstats

On Thu, Feb 24, 2022 at 9:05 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Feb 24, 2022 at 2:24 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > 10. src/backend/replication/logical/worker.c
> >
> > (from my previous [Peter-v1] #9)
> >
> > >> + /* Report the worker failed during table synchronization */
> > >> + pgstat_report_subscription_error(MyLogicalRepWorker->subid, false);
> > >>
> > >> and
> > >>
> > >> + /* Report the worker failed during the application of the change */
> > >> + pgstat_report_subscription_error(MyLogicalRepWorker->subid, true);
> > >>
> > >>
> > >> Why don't these use MySubscription->oid instead of MyLogicalRepWorker->subid?
> >
> > > It's just because we used to use MyLogicalRepWorker->subid, is there
> > > any particular reason why we should use MySubscription->oid here?
> >
> > I felt MySubscription->oid is a more natural and more direct way of
> > expressing the same thing.
> >
> > Consider: "the oid of the current subscription" versus "the oid of
> > the subscription of the current worker". IMO the first one is simpler.
> > YMMV.
> >
>
> I think we can use either but maybe MySubscription->oid would be
> slightly better here as the same is used in nearby code as well.

Okay, will change.

> >
> > 13. src/test/subscription/t/026_worker_stats.pl - missing test?
> >
> > Shouldn't there also be some test to reset the counters to confirm
> > that they really do get reset to zero?
> >
> > ~~~
> >
>
> I think we avoid writing tests for stats for each and every case as it
> is not reliable in nature (the message can be lost). If we can find a
> reliable way then it is okay.

Yeah, the messages can even be out-of-order. Particularly, in this
test, the apply worker and table sync worker keep reporting the
messages, it's quite possible that the test becomes unstable. I
remember we removed unstable tests of resetting statistics before
(e.g., see fc6950913).

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/