Re: Skipping logical replication transactions on subscriber side

On Sun, Nov 7, 2021 at 7:50 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Nov 3, 2021 at 12:41 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Nov 2, 2021 at 2:17 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > If we follow the idea of keeping stats at db level (in
> > PgStat_StatDBEntry) as discussed above then I think we already have a
> > way to remove stat entries via pg_stat_reset which removes the stats
> > corresponding to tables, functions and after this patch corresponding
> > to subscriptions as well for the current database. Won't that be
> > sufficient? I see your point but I think it may be better if we keep
> > the same behavior for stats of apply and table sync workers.
>
> Make sense.
>

We can document this point.

> >
> > Following the tables, functions, I thought of keeping the name of the
> > reset function similar to "pg_stat_reset_single_table_counters" but I
> > feel the currently used name "pg_stat_reset_subscription_worker" in
> > the patch is better. Do let me know what you think?
>
> Yeah, I also tend to prefer pg_stat_reset_subscription_worker name
> since "single" isn't clear in the context of subscription worker. And
> the behavior of the reset function for subscription workers is also
> different from pg_stat_reset_single_xxx_counters.
>
> I've attached an updated patch. In this version patch, subscription
> worker statistics are collected per-database and handled in a similar
> way to tables and functions. I think perhaps we still need to discuss
> details of how the statistics should be handled but I'd like to share
> the patch for discussion.
>

Do you have something specific in mind to discuss the details of how
stats should be handled?

Few comments/questions:
====================
1.
static void pgstat_reset_replslot(PgStat_StatReplSlotEntry
*slotstats, TimestampTz ts);

+
static void pgstat_send_tabstat(PgStat_MsgTabstat *tsmsg, TimestampTz now);

Spurious line addition.

2. Why now there is no code to deal with dead table sync entries as
compared to previous version of patch?

3. Why do we need two different functions
pg_stat_reset_subscription_worker_sub and
pg_stat_reset_subscription_worker_subrel to handle reset? Isn't it
sufficient to reset all entries for a subscription if relid is
InvalidOid?

4. It seems now stats_reset entry is not present in
pg_stat_subscription_workers? How will users find that information if
required?

--
With Regards,
Amit Kapila.