On Thursday, February 24, 2022 11:07 AM Masahiko Sawada <sawada.mshk@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
