On Mon, Feb 28, 2022 at 11:33 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Sat, Feb 26, 2022 at 1:35 PM osumi.takami...@fujitsu.com > <osumi.takami...@fujitsu.com> wrote: > > > > On Saturday, February 26, 2022 11:51 AM Amit Kapila > > <amit.kapil...@gmail.com> wrote: > > > I have reviewed the latest version and made a few changes along with > > > fixing > > > some of the pending comments by Peter Smith. The changes are as > > > follows: (a) Removed m_databaseid in PgStat_MsgSubscriptionError as that > > > is > > > not required now; (b) changed the struct name PgStat_MsgSubscriptionPurge > > > to PgStat_MsgSubscriptionDrop to make it similar to DropDb; (c) changed > > > the > > > view name to pg_stat_subscription_stats, we can reconsider it in future > > > if there > > > is a consensus on some other name, accordingly changed the reset function > > > name to pg_stat_reset_subscription_stats; (d) moved some of the newly > > > added subscription stats functions adjacent to slots to main the > > > consistency in > > > code; (e) changed comments at few places; (f) added LATERAL back to > > > system_views query as we refer pg_subscription's oid in the function call, > > > previously that was not clear. > > > > > > Do let me know what you think of the attached? > > Hi, thank you for updating the patch ! > > > > > > I have a couple of comments on v4. > > > > (1) > > > > I'm not sure if I'm correct, but I'd say the sync_error_count > > can come next to the subname as the order of columns. > > I felt there's case that the column order is somewhat > > related to the time/processing order (I imagined > > pg_stat_replication's LSN related columns). > > If this was right, table sync related column could be the > > first column as a counter within this patch. > > > > I am not sure if there is such a correlation but even if it is there > it doesn't seem to fit here completely as sync errors can happen after > apply errors in multiple ways like via Alter Subscription ... Refresh > ... > > So, I don't see the need to change the order here. What do you or others > think?
I'm also not sure about it, both sound good to me. Probably we can change the order later. > > > > > (2) doc/src/sgml/monitoring.sgml > > > > + Resets statistics for a single subscription shown in the > > + <structname>pg_stat_subscription_stats</structname> view to zero. > > If > > + the argument is <literal>NULL</literal>, reset statistics for all > > + subscriptions. > > </para> > > > > I felt we could improve the first sentence. > > > > From: > > Resets statistics for a single subscription shown in the.. > > > > To(idea1): > > Resets statistics for a single subscription defined by the argument to zero. > > > > Okay, I can use this one. Are you going to remove the part "shown in the pg_stat_subsctiption_stats view"? I think it's better to keep it in order to make it clear which statistics the function resets as we have pg_stat_subscription and pg_stat_subscription_stats. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
