Re: Failed transaction statistics to measure the logical replication progress

On Mon, Feb 21, 2022 at 12:45 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Saturday, February 19, 2022 12:00 AM osumi(dot)takamichi(at)fujitsu(dot)com <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> > On Friday, February 18, 2022 3:34 PM Tang, Haiying/唐 海英
> > <tanghy(dot)fnst(at)fujitsu(dot)com> wrote:
> > > On Wed, Jan 12, 2022 8:35 PM osumi(dot)takamichi(at)fujitsu(dot)com
> > > <osumi(dot)takamichi(at)fujitsu(dot)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/