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
