RE: Failed transaction statistics to measure the logical replication progress

On Wed, Jan 12, 2022 8:35 PM osumi(dot)takamichi(at)fujitsu(dot)com <osumi(dot)takamichi(at)fujitsu(dot)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