Re: Failed transaction statistics to measure the logical replication progress

On Mon, Dec 20, 2021 at 8:40 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)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