From: | "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com> |
---|---|
To: | 'Masahiko Sawada' <sawada(dot)mshk(at)gmail(dot)com> |
Cc: | "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, "wangw(dot)fnst(at)fujitsu(dot)com" <wangw(dot)fnst(at)fujitsu(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, "vignesh21(at)gmail(dot)com" <vignesh21(at)gmail(dot)com>, "amit(dot)kapila16(at)gmail(dot)com" <amit(dot)kapila16(at)gmail(dot)com>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Greg Nancarrow <gregn4422(at)gmail(dot)com> |
Subject: | RE: Failed transaction statistics to measure the logical replication progress |
Date: | 2022-02-24 22:57:39 |
Message-ID: | TYCPR01MB8373A3E1BE237BAF38185BF2ED3D9@TYCPR01MB8373.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thursday, February 24, 2022 11:07 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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.
Best Regards,
Takamichi Osumi
Attachment | Content-Type | Size |
---|---|---|
v24-0001-Extend-pg_stat_subscription_workers-to-include-g.patch | application/octet-stream | 20.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | osumi.takamichi@fujitsu.com | 2022-02-24 23:01:57 | RE: Failed transaction statistics to measure the logical replication progress |
Previous Message | Tomas Vondra | 2022-02-24 22:22:31 | Re: ltree_gist indexes broken after pg_upgrade from 12 to 13 |