On Mon, Feb 21, 2022 at 12:45 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Saturday, February 19, 2022 12:00 AM osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com> wrote:
> > On Friday, February 18, 2022 3:34 PM Tang, Haiying/唐 海英
> > <tanghy.fnst@fujitsu.com> wrote:
> > > On Wed, Jan 12, 2022 8:35 PM osumi.takamichi@fujitsu.com
> > > <osumi.takamichi@fujitsu.com> wrote:
> > > 4) I noticed that the abort_count doesn't include aborted streaming
> > > transactions.
> > > Should we take this case into consideration?
> > Hmm, we can add this into this column, when there's no objection.
> > I'm not sure but someone might say those should be separate columns.
> I've addressed this point in a new v23 patch,
> since there was no opinion on this so far.
>
> Kindly have a look at the attached one.
>
I have some comments on v23 patch:
@@ -66,6 +66,12 @@ typedef struct LogicalRepWorker
TimestampTz last_recv_time;
XLogRecPtr reply_lsn;
TimestampTz reply_time;
+
+ /*
+ * Transaction statistics of subscription worker
+ */
+ int64 commit_count;
+ int64 abort_count;
} LogicalRepWorker;
I think that adding these statistics to the struct whose data is
allocated on the shared memory is not a good idea since they don't
need to be shared. We might want to add more statistics for
subscriptions such as insert_count and update_count in the future. I
think it's better to track these statistics in local memory either in
worker.c or pgstat.c.
+/* ----------
+ * pgstat_report_subworker_xact_end() -
+ *
+ * Update the statistics of subscription worker and have
+ * pgstat_report_stat send a message to stats collector
+ * after count increment.
+ * ----------
+ */
+void
+pgstat_report_subworker_xact_end(bool is_commit)
+{
+ if (is_commit)
+ MyLogicalRepWorker->commit_count++;
+ else
+ MyLogicalRepWorker->abort_count++;
+}
It's slightly odd and it seems unnecessary to me that we modify fields
of MyLogicalRepWorker in pgstat.c. Although this function has “report”
in its name but it just increments the counter. I think we can do that
in worker.c.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
