RE: Failed transaction statistics to measure the logical replication progress

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