Sorry for the late reply. I was on vacation. On Tue, Sep 14, 2021 at 11:27 AM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> wrote: > > From Thur, Sep 9, 2021 10:33 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > Sorry for the late response. I've attached the updated patches that > > incorporate > > all comments unless I missed something. Please review them. > > Thanks for the new version patches. > Here are some comments for the v13-0001 patch.
Thank you for the comments! > > 1) > > + pgstat_setheader(&errmsg.m_hdr, > PGSTAT_MTYPE_SUBSCRIPTIONERRPURGE); > + pgstat_send(&errmsg, len); > + errmsg.m_nentries = 0; > + } > > It seems we can invoke pgstat_setheader once before the loop like the > following: > > + errmsg.m_nentries = 0; > + errmsg.m_subid = subent->subid; > + pgstat_setheader(&errmsg.m_hdr, > PGSTAT_MTYPE_SUBSCRIPTIONERRPURGE); > > 2) > + pgstat_setheader(&submsg.m_hdr, > PGSTAT_MTYPE_SUBSCRIPTIONPURGE); > + pgstat_send(&submsg, len); > > Same as 1), we can invoke pgstat_setheader once before the loop like: > + submsg.m_nentries = 0; > + pgstat_setheader(&submsg.m_hdr, > PGSTAT_MTYPE_SUBSCRIPTIONPURGE); > But if we do that, we set the header even if there is no message to send, right? Looking at other similar code in pgstat_vacuum_stat(), we set the header just before sending the message. So I'd like to leave them since it's cleaner. > > 3) > > +/* ---------- > + * PgStat_MsgSubscriptionErrPurge Sent by the autovacuum to purge the > subscription > + * > errors. > > The comments said it's sent by autovacuum, would the manual vacuum also send > this message ? Right. Fixed. > > > 4) > + > + pgstat_send(&msg, offsetof(PgStat_MsgSubscriptionErr, m_reset) + > sizeof(bool)); > +} > > Does it look cleaner that we use the offset of m_relid here like the > following ? > > pgstat_send(&msg, offsetof(PgStat_MsgSubscriptionErr, m_relid)); Thank you for the suggestion. After more thought, it was a bit odd to use PgStat_MsgSubscriptionErr to both report and reset the stats by sending the part or the full struct. So in the latest version, I've added a new message struct type to reset the subscription error statistics. I've attached the updated version patches. Please review them. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
v14-0002-Add-RESET-command-to-ALTER-SUBSCRIPTION-command.patch
Description: Binary data
v14-0003-Add-skip_xid-option-to-ALTER-SUBSCRIPTION.patch
Description: Binary data
v14-0001-Add-pg_stat_subscription_errors-statistics-view.patch
Description: Binary data
