On Monday, December 13, 2021 6:19 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > On Tue, Dec 7, 2021 at 3:12 PM osumi.takami...@fujitsu.com > <osumi.takami...@fujitsu.com> wrote: > > > > Few questions and comments: > ======================== > 1. > The <structname>pg_stat_subscription_workers</structname> view will > contain > one row per subscription worker on which errors have occurred, for workers > applying logical replication changes and workers handling the initial data > - copy of the subscribed tables. The statistics entry is removed when the > - corresponding subscription is dropped. > + copy of the subscribed tables. Also, the row corresponding to the apply > + worker shows all transaction statistics of both types of workers on the > + subscription. The statistics entry is removed when the corresponding > + subscription is dropped. > > Why did you choose to show stats for both types of workers in one row? Now, the added stats show only the statistics of apply worker as we agreed.
> 2. > + PGSTAT_MTYPE_SUBWORKERXACTEND, > } StatMsgType; > > I don't think we comma with the last message type. Fixed. > 3. > + Oid m_subrelid; > + > + /* necessary to determine column to increment */ LogicalRepMsgType > + m_command; > + > +} PgStat_MsgSubWorkerXactEnd; > > Is m_subrelid used in this patch? If not, why did you keep it? I think if you > choose to show separate stats for table sync and apply worker then probably it > will be used. Removed. > 4. > /* > + * Cumulative transaction statistics of subscription worker */ > + PgStat_Counter commit_count; PgStat_Counter error_count; > + PgStat_Counter abort_count; > + > > I think it is better to keep the order of columns as commit_count, > abort_count, > error_count in the entire patch. Fixed. The new patch is shared in [1]. [1] - https://www.postgresql.org/message-id/TYCPR01MB83734A7A0596AC7ADB0DCB51ED779%40TYCPR01MB8373.jpnprd01.prod.outlook.com Best Regards, Takamichi Osumi