Re: Design of pg_stat_subscription_workers vs pgstats

On Thu, Feb 24, 2022 at 2:24 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 9. src/backend/postmaster/pgstat.c - pgstat_recv_subscription_purge
>
> static void
> pgstat_recv_subscription_purge(PgStat_MsgSubscriptionPurge *msg, int len)
> {
> /* Return if we don't have replication subscription statistics */
> if (subscriptionStatHash == NULL)
> return;
>
> /* Remove from hashtable if present; we don't care if it's not */
> (void) hash_search(subscriptionStatHash, (void *) &(msg->m_subid),
> HASH_REMOVE, NULL);
> }
>
> SUGGESTION
> Wouldn't the above code be simpler written like:
>
> if (subscriptionStatHash)
> {
> /* Remove from hashtable if present; we don't care if it's not */
> (void) hash_search(subscriptionStatHash, (void *) &(msg->m_subid),
> HASH_REMOVE, NULL);
> }
> ~~~
>

I think we can write that way as well but I would prefer the way it is
currently in the patch as we use a similar pattern in nearby code (ex.
pgstat_recv_resetreplslotcounter) and at other places in the code base
as well.

> 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.

> Also, it is shorter :)
>
> ~~~
>
> 11. src/include/pgstat.h - enum order
>
> (follow-on from my previous v1 review comment #10)
>
> >I assume you're concerned about binary compatibility or something. I
> >think it should not be a problem since both
> >PGSTAT_MTYPE_SUBWORKERERROR and PGSTAT_MTYPE_SUBSCRIPTIONPURGE are
> >introduced to PG15.
>
> Yes, maybe it is OK for those ones. But now in v2 there is a new
> PGSTAT_MTYPE_RESETSUBCOUNTER.
>
> Shouldn't at least that one be put at the end for the same reason?
>
> ~~~
>

I don't see the reason to put that at end. It is better to add it near
to similar RESET enums.

>
> 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.

--
With Regards,
Amit Kapila.