On Thursday, February 24, 2022 11:07 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > 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. Fixed.
> +/* ----------
> + * 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.
Fixed.
Also, I made the timing adjustment logic
back and now have the independent one as Amit-san suggested in [1].
Kindly have a look at v24.
[1] -
https://www.postgresql.org/message-id/CAA4eK1LWYc15%3DASj1tMTEFsXtxu%3D02aGoMwq9YanUVr9-QMhdQ%40mail.gmail.com
Best Regards,
Takamichi Osumi
v24-0001-Extend-pg_stat_subscription_workers-to-include-g.patch
Description: v24-0001-Extend-pg_stat_subscription_workers-to-include-g.patch
