Re: Failed transaction statistics to measure the logical replication progress

On Tue, Dec 7, 2021 at 3:12 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)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?

2.
+ PGSTAT_MTYPE_SUBWORKERXACTEND,
} StatMsgType;

I don't think we comma with the last message type.

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.

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.

--
With Regards,
Amit Kapila.