On Sat, Feb 26, 2022 at 1:35 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Saturday, February 26, 2022 11:51 AM Amit Kapila <amit.kapila16@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?
>
> (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.
--
With Regards,
Amit Kapila.
