Re: Failed transaction statistics to measure the logical replication progress

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

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.