On Tuesday, December 21, 2021 6:00 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > Some review comments on the v18 patches: Thank you for your review !
> 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."
Fixed.
> 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
Fixed.
> 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.
Fixed.
> (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))
>
> ?
I'm not sure which is better and
I never have strong objection to your idea but currently
I prefer the previous code because we don't need to
add one extra condition (last_report != 0) in the function called really
frequently
and the optimization to avoid calling TimestampDifferenceExceeds works just once
with your change, I'd say.
We call pgstat_send_subworker_xact_stats() in the LogicalRepApplyLoop's loop.
For the apply worker, this should be the first call for normal operation,
before any call of the apply_dispatch(and subsequent commit-like functions which
calls pgstat_send_subworker_xact_stats() in the end).
In the first call, existing v18's codes (without your suggested change) just
initializes
'last_report' variable because of the diff between 0 and 'now' and returns
because of no stats values in commit_count and abort_count in the function.
After this, 'last_report' will not be 0 for the apply worker.
On the other hand, in the case I add your change, in the first call of
pgstat_send_subworker_xact_stats(), similarly 'last_report' is initialized but
without one call of TimestampDifferenceExceeds(), which might be the
optimization
effect and the function returns with no stats again. Here the 'last_report' will
not be 0 after this. But, then we'll have to check the condition in the apply
worker
in the every loop. Besides, after the first setting of 'last_report',
every call of the pgstat_send_subworker_xact_stats() calculates the time
subtraction.
This means the one skipped call of the function looks less worth in the case of
frequent
calls of the function. So, I'm not sure it' a good idea to incorporate this
change.
Kindly let me know if I miss something.
At present, I'll keep the code as it is.
> (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."
Fixed.
Attached the new patch v19.
Best Regards,
Takamichi Osumi
v19-0002-Extend-pg_stat_subscription_workers-to-include-g.patch
Description: v19-0002-Extend-pg_stat_subscription_workers-to-include-g.patch
v19-0001-Fix-alignments-of-TAP-tests-for-pg_stat_subscrip.patch
Description: v19-0001-Fix-alignments-of-TAP-tests-for-pg_stat_subscrip.patch
