RE: Design of pg_stat_subscription_workers vs pgstats

From: "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com>
To: 'Masahiko Sawada' <sawada(dot)mshk(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Andres Freund <andres(at)anarazel(dot)de>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: RE: Design of pg_stat_subscription_workers vs pgstats
Date: 2022-02-22 12:22:18
Message-ID: TYCPR01MB8373D12532FEECDAFDD5BA2BED3B9@TYCPR01MB8373.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Aleksander Alekseev 2022-02-22 13:31:04 Re: External data files possible?
Previous Message Marcos Pegoraro 2022-02-22 12:00:36 Re: Support logical replication of DDLs