RE: Design of pg_stat_subscription_workers vs pgstats

On Tuesday, February 22, 2022 2:45 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> 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