Re: Skipping logical replication transactions on subscriber side

On Tue, Nov 9, 2021 at 3:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Nov 7, 2021 at 7:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Nov 3, 2021 at 12:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Nov 2, 2021 at 2:17 PM Masahiko Sawada <sawada.mshk@gmail.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.

Okay.

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

As you commented, I removed stats_reset column from
pg_stat_subscription_workers view since tables and functions stats
view doesn't have it.

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

Will fix.

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

I think we discussed that it's better if we keep the same behavior for
stats of apply and table sync workers. So the table sync entries are
dead after the subscription is dropped, like apply entries. No?


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

Since setting InvalidOid to relid means an apply entry we cannot use
it for that purpose.

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

Users can find it in pg_stat_databases. The same is true for table and
function statistics -- they don't have stats_reset column but reset
stats_reset of its entry on pg_stat_database.

Regards,

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