On Thu, Feb 24, 2022 at 9:05 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Feb 24, 2022 at 2:24 PM Peter Smith <smithpb2...@gmail.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/
