RE: Failed transaction statistics to measure the logical replication progress

On Wed, Jan 12, 2022 8:35 PM osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com> wrote:
> 
> The attached v21 has a couple of other minor updates
> like a modification of error message text.
> 
> 

Thanks for updating the patch. Here are some comments.

1) I saw the following description about pg_stat_subscription_workers view in
existing doc:

   The <structname>pg_stat_subscription_workers</structname> view will contain
   one row per subscription worker on which errors have occurred, ...

It only says  "which errors have occurred", maybe we should also mention
transactions here, right?

2)
/* ----------
+ * pgstat_send_subworker_xact_stats() -
+ *
+ *    Send a subworker's transaction stats to the collector.
+ *    The statistics are cleared upon return.

Should "The statistics are cleared upon return" changed to "The statistics are
cleared upon sending"? Because if it doesn't reach PGSTAT_STAT_INTERVAL and the
transaction stats are not sent, the function will return without clearing out
statistics.

3)
+    Assert(command == LOGICAL_REP_MSG_COMMIT ||
+           command == LOGICAL_REP_MSG_STREAM_COMMIT ||
+           command == LOGICAL_REP_MSG_COMMIT_PREPARED ||
+           command == LOGICAL_REP_MSG_ROLLBACK_PREPARED);
+
+    switch (command)
+    {
+        case LOGICAL_REP_MSG_COMMIT:
+        case LOGICAL_REP_MSG_STREAM_COMMIT:
+        case LOGICAL_REP_MSG_COMMIT_PREPARED:
+            MyLogicalRepWorker->commit_count++;
+            break;
+        case LOGICAL_REP_MSG_ROLLBACK_PREPARED:
+            MyLogicalRepWorker->abort_count++;
+            break;
+        default:
+            ereport(ERROR,
+                    errmsg("invalid logical message type for transaction statistics of subscription"));
+            break;
+    }

I'm not sure that do we need the assert, because it will report an error later
if command is an invalid value.

4) I noticed that the abort_count doesn't include aborted streaming transactions.
Should we take this case into consideration?

Regards,
Tang