On Mon, Dec 20, 2021 at 8:40 PM osumi.takami...@fujitsu.com <osumi.takami...@fujitsu.com> wrote: > > Updated the patch to address your concern. >
Some review comments on the v18 patches: v18-0002 doc/src/sgml/monitoring.sgml (1) tablesync worker stats? Shouldn't the comment below only mention the apply worker? (since we're no longer recording stats of the tablesync worker) + Number of transactions that failed to be applied by the table + sync worker or main apply worker in this subscription. This + counter is updated after confirming the error is not same as + the previous one. + </para></entry> Also, it should say "... the error is not the same as the previous one." src/backend/catalog/system_views.sql (2) pgstat_report_subworker_xact_end() Fix typo and some wording: BEFORE: + * This should be called before the call of process_syning_tables() not to AFTER: + * This should be called before the call of process_syncing_tables(), so to not src/backend/postmaster/pgstat.c (3) pgstat_send_subworker_xact_stats() BEFORE: + * Send a subworker transaction stats to the collector. AFTER: + * Send a subworker's transaction stats to the collector. (4) Wouldn't it be best for: + if (!TimestampDifferenceExceeds(last_report, now, PGSTAT_STAT_INTERVAL)) to be: + if (last_report != 0 && !TimestampDifferenceExceeds(last_report, now, PGSTAT_STAT_INTERVAL)) ? (5) pgstat_send_subworker_xact_stats() I think that the comment: + * Clear out the statistics buffer, so it can be re-used. should instead say: + * Clear out the supplied statistics. because the current comment infers that repWorker is pointed at the MyLogicalRepWorker buffer (which it might be but it shouldn't be relying on that) Also, I think that the function header should mention something like: "The supplied repWorker statistics are cleared upon return, to assist re-use by the caller." Regards, Greg Nancarrow Fujitsu Australia
