On Tuesday, February 22, 2022 2:45 PM Masahiko Sawada <[email protected]>
wrote:
> I've attached a patch that changes pg_stat_subscription_workers view.
> It removes non-cumulative values such as error details such as error-XID and
> the error message from the view, and consequently the view now has only
> cumulative statistics counters: apply_error_count and sync_error_count. Since
> the new view name is under discussion I temporarily chose
> pg_stat_subscription_activity.
Hi, thank you for sharing the patch.
Few minor comments for v1.
(1) commit message's typo
This commits changes the view so that it stores only statistics
counters: apply_error_count and sync_error_count.
"This commits" -> "This commit"
(2) minor improvement suggestion for the commit message
I suggest that we touch the commit id 8d74fc9
that introduces the pg_stat_subscription_workers
in the commit message, for better traceability. Below is an example.
From:
As the result of the discussion, we've concluded that the stats
collector is not an appropriate place to store the error information of
subscription workers.
To:
As the result of the discussion about the view introduced by 8d74fc9,...
(3) doc/src/sgml/logical-replication.sgml
Kindly refer to commit id 85c61ba for the detail.
You forgot "the" in the below sentence.
@@ -346,8 +346,6 @@
<para>
A conflict will produce an error and will stop the replication; it must be
resolved manually by the user. Details about the conflict can be found in
- <link linkend="monitoring-pg-stat-subscription-workers">
- <structname>pg_stat_subscription_workers</structname></link> and the
subscriber's server log.
</para>
From:
subscriber's server log.
to:
the subscriber's server log.
(4) doc/src/sgml/monitoring.sgml
<row>
<entry role="catalog_table_entry"><para role="column_definition">
- <structfield>last_error_time</structfield> <type>timestamp with time
zone</type>
+ <structfield>sync_error_count</structfield> <type>uint8</type>
</para>
<para>
- Last time at which this error occurred
+ Number of times the error occurred during the initial data copy
</para></entry>
I supposed it might be better to use "initial data sync"
or "initial data synchronization", rather than "initial data copy".
(5) src/test/subscription/t/026_worker_stats.pl
+# Truncate test_tab1 so that table sync can continue.
+$node_subscriber->safe_psql('postgres', "TRUNCATE test_tab1;");
The second truncate is for apply, isn't it? Therefore, kindly change
From:
Truncate test_tab1 so that table sync can continue.
To:
Truncate test_tab1 so that apply can continue.
(6) src/test/subscription/t/026_worker_stats.pl
+# Insert more data to test_tab1 on the subscriber and then on the publisher,
raising an
+# error on the subscriber due to violation of the unique constraint on
test_tab1.
+$node_subscriber->safe_psql('postgres', "INSERT INTO test_tab1 VALUES (2)");
Did we need this insert ?
If you want to indicate the apply is working okay after the error of table sync
is solved,
waiting for the max value in the test_tab1 becoming 2 on the subscriber by
polling query
would work. But, I was not sure if this is essentially necessary for the
testing purpose.
Best Regards,
Takamichi Osumi
