public inbox for [email protected]
help / color / mirror / Atom feedFrom: Greg Nancarrow <[email protected]>
To: [email protected] <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Failed transaction statistics to measure the logical replication progress
Date: Tue, 21 Dec 2021 19:59:34 +1100
Message-ID: <CAJcOf-dmfinJDr=nOmjX1qNiU_mvW5tG3GyVwsQzRnUBb2ky0A@mail.gmail.com> (raw)
In-Reply-To: <TYCPR01MB83736371B66F90C71365ED11ED7B9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
References: <CALDaNm0Wg8XwfqUVnBSSahOtNxGDLNOW1_tc4ikZK=tGfqje-A@mail.gmail.com>
<TYCPR01MB8373A9B6508E44E547A98EF5ED779@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<TYCPR01MB83734A7A0596AC7ADB0DCB51ED779@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<[email protected]>
<TYCPR01MB83736371B66F90C71365ED11ED7B9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
On Mon, Dec 20, 2021 at 8:40 PM [email protected]
<[email protected]> 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
view thread (113+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Failed transaction statistics to measure the logical replication progress
In-Reply-To: <CAJcOf-dmfinJDr=nOmjX1qNiU_mvW5tG3GyVwsQzRnUBb2ky0A@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox