Subject: Re: Design of pg_stat_subscription_workers vs pgstats

From: Andres Freund <[email-masked]>
Date: Sun, 20 Feb 2022 21:34:53 -0800

Hi,

On 2022-02-21 12:56:46 +0900, Masahiko Sawada wrote:
> > The patch you referenced [1] should just store the stats in the
> > pg_stat_subscription view, not pg_stat_subscription_workers.
> >
> > It *does* make sense to keep stats about the number of table syncs that failed
> > etc. But that should be a counter in pg_stat_subscription, not a row in
> > pg_stat_subscription_workers.
>
> We have discussed using pg_stat_subscription before but concluded it's
> not an appropriate place to store error information because it ends up
> keeping cumulative stats mixed with non-cumulative stats.

Well, as we've amply discussed, the non-cumulative stats shouldn't be in the
pgstat subsystem.

> To take a precedent, we used to store accumulative statistics such as
> spill_txns to pg_stat_replication, but then for the same reason we moved
> those statistics to the new stats view, pg_stat_replication_slot. New
> subscription statistics that we're introducing are cumulative statistics
> whereas pg_stat_subscription is a dynamic statistics view.

I'm happy to have cumulative subscriber stats somewhere in pgstats. But it
shouldn't be split by worker or relation, and it shouldn't contain
non-cumulative error information.

Greetings,

Andres Freund

---

From: Amit Kapila <[email-masked]>
Date: Mon, 21 Feb 2022 12:39:31 +0530

> non-cumulative error information.

Fair enough. Then, how about the following keeping the following information:

* subid (subscription id)
* subname (subscription name)
* sync_error_count/sync_failure_count (number of timed table sync failed)
* apply_error_count/apply_failure_count (number of times apply failed)
* sync_success_count (number of table syncs successfully finished)
* apply_commit_count (number of transactions applied successfully)
* apply_rollback_count (number of transactions explicitly rolled back)
* stats_reset (Time at which these statistics were last reset)

The view name could be pg_stat_subscription_lrep,
pg_stat_logical_replication, or something on those lines.

-- 
With Regards,
Amit Kapila.

---

From: Andres Freund <[email-masked]>
Date: Sun, 20 Feb 2022 23:48:06 -0800

Hi,

> Fair enough. Then, how about the following keeping the following information:

Mostly sounds good.

> * subname (subscription name)

Coming from catalog, via join, I assume?

> * apply_error_count/apply_failure_count (number of times apply failed)

Yep.

> * sync_success_count (number of table syncs successfully finished)

This one I'm not quite convinced by. You can't rely on precise counts with
pgstats and we should be able to get a better idea from somewhere more
permanent which relations succeeded?  But it also doesn't do much harm, so ...

> * apply_rollback_count (number of transactions explicitly rolled back)

What does "explicit" mean here?

> pg_stat_logical_replication, or something on those lines.

pg_stat_subscription_stats :)

(I really dislike that we have pg_stat_ stuff that's not actually stats, but
something describing the current state, but that ship has well sailed).

Greetings,

Andres Freund

---

From: Amit Kapila <[email-masked]>
Date: Mon, 21 Feb 2022 14:49:01 +0530

> Coming from catalog, via join, I assume?

The subname would be from pg_subscription catalog similar to what we
are doing now for pg_stat_subscription_workers.

> permanent which relations succeeded?  But it also doesn't do much harm, so ...

We can get precise information from pg_subscription_rel (rels that are
in ready/finish_copy state) but OTOH, during refresh some of the rels
would have been dropped or if a user creates/refreshes publication
with copy_data = false, then we won't get information about how many
table syncs succeeded? I have also kept this to make the sync
information look consistent considering we have sync_failure_count.

> What does "explicit" mean here?

It is for the Rollback Prepared case and probably for streaming of
in-progress transactions that eventually get rolled back.

> pg_stat_subscription_stats :)

Having *stat* two times in the name sounds slightly odd to me but let
us see what others think. One more option could be
pg_stat_subscription_replication.

-- 
With Regards,
Amit Kapila.

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 21 Feb 2022 09:07:07 -0700

> pg_stat_subscription_replication.
Agreed.

pg_stat_subscription_activity

We already have pg_stat_activity (which may be an argument against the
suggestion...)

David J.

---

From: Andres Freund <[email-masked]>
Date: Mon, 21 Feb 2022 08:17:15 -0800

Hi,

> pg_stat_subscription_replication.

It was a joke, making light of our bad naming in pg_stat_*, not a serious
suggestion...

Greetings,

Andres Freund

---

From: Amit Kapila <[email-masked]>
Date: Tue, 22 Feb 2022 09:07:30 +0530

On Mon, Feb 21, 2022 at 9:37 PM David G. Johnston
<[email-masked]> wrote:
> We already have pg_stat_activity (which may be an argument against the suggestion...)
>

I don't know if that can be an argument against it but one can imagine
that we record other subscription changes like (change of
publications, etc.). I personally feel it may be better to add
'_replication' in some way like pg_stat_sub_replication_activity but I
am fine either way.

--
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 22 Feb 2022 14:45:19 +0900

> are doing now for pg_stat_subscription_workers.

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.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

---

From: Amit Kapila <[email-masked]>
Date: Tue, 22 Feb 2022 15:23:38 +0530

> temporarily chose pg_stat_subscription_activity.

Few comments:
=============
1.
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -637,11 +637,9 @@ REVOKE EXECUTE ON FUNCTION
pg_stat_reset_single_table_counters(oid) FROM public;

 REVOKE EXECUTE ON FUNCTION
pg_stat_reset_single_function_counters(oid) FROM public;

-REVOKE EXECUTE ON FUNCTION pg_stat_reset_replication_slot(text) FROM public;
-
-REVOKE EXECUTE ON FUNCTION pg_stat_reset_subscription_worker(oid) FROM public;
+REVOKE EXECUTE ON FUNCTION
pg_stat_reset_single_subscription_counters(oid) FROM public;

-REVOKE EXECUTE ON FUNCTION pg_stat_reset_subscription_worker(oid,
oid) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_stat_reset_replication_slot(text) FROM public;

Is there a need to change anything about
pg_stat_reset_replication_slot() in this patch?

2. Do we still need to use LATERAL in the view's query?

3. Can we send error stats pgstat_report_stat() as that will be called
via proc_exit() path. We can set the phase (apply/sync) in
apply_error_callback_arg and then use that to send the appropriate
message. I think this will obviate the need for try..catch.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 22 Feb 2022 12:22:18 +0000

On Tuesday, February 22, 2022 2:45 PM Masahiko Sawada <[email-masked]> wrote:
> 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

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 22 Feb 2022 23:32:02 +0900

> > temporarily chose pg_stat_subscription_activity.

Thank you for the comments!

> pg_stat_reset_replication_slot() in this patch?

It doesn't change pg_stat_reset_replication_slot() but just changes
the order in order to put the modified function
pg_stat_reset_single_subscription_counters() closer to other similar
functions such as pg_stat_reset_single_function_counters().

> 2. Do we still need to use LATERAL in the view's query?

There are some functions that use LATERAL in a similar way but it
seems no need to put LATERAL before the function call. Will remove.

> message. I think this will obviate the need for try..catch.

If we use pgstat_report_stat() to send subscription stats messages,
all processes end up going through that path. It might not bring
overhead in practice but I'd like to avoid it. And, since the apply
worker also calls pgstat_report_stat() at the end of the transaction,
we might need to change pgstat_report_stat() so that it doesn't send
the subscription messages when it gets called at the end of the
transaction. I think it's likely that PG_TRY() and PG_CATCH() wil be
added for example, when the disable_on_error feature or the storing
error details feature is introduced, so obviating the need for them at
this point would not benefit much.

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 22 Feb 2022 23:46:31 +0900

> "This commits" -> "This commit"

Will fix.

> As the result of the discussion about the view introduced by 8d74fc9,...

Okay, will add the commit reference.

> You forgot "the" in the below sentence.

Will fix.

> or "initial data synchronization", rather than "initial data copy".

"Initial data synchronization" sounds like the whole table
synchronization process including COPY and applying changes to catch
up. But sync_error_count is incremented only during COPY so I used
"initial data copy". What do you think?

> Truncate test_tab1 so that apply can continue.

Right, will fix.

> would work. But, I was not sure if this is essentially necessary for the testing purpose.

You're right, it's not necessary. Also, it seems better to change the
TAP test file name from 026_worker_stats.pl to 026_stats.pl. Will
incorporate these changes.

Regards,

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 23 Feb 2022 01:13:45 +0000

On Tuesday, February 22, 2022 11:47 PM Masahiko Sawada <[email-masked]> wrote:
> > +       Number of times the error occurred during the initial data
> > + copy
> >        </para></entry>
> >
> "Initial data synchronization" sounds like the whole table synchronization
> process including COPY and applying changes to catch up. But
> sync_error_count is incremented only during COPY so I used "initial data copy".
> What do you think?
Okay. Please keep it as is.

Best Regards,
	Takamichi Osumi

---

From: Andres Freund <[email-masked]>
Date: Tue, 22 Feb 2022 18:14:15 -0800

Hi,

> I've attached a patch that changes pg_stat_subscription_workers view.

Thanks for working on this!

Why are the stats stored in the per-database stats file / as a second level
below the database? While they're also associated with a database, it's a
global catalog, so it seems to make more sense to have them "live" globally as
well?

Not just from an aesthetical perspective, but there might also be cases where
it's useful to send stats from the stats launcher. E.g. the number of times
the launcher couldn't start a worker because the max numbers of workers was
already active or such.

Greetings,

Andres Freund

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 23 Feb 2022 11:46:14 +0900

> global catalog, so it seems to make more sense to have them "live" globally as

Good point. The reason why we used to use per-database stats file is
that we were storing some relation information there. But now that we
don't need to have such information, it makes more sense to have them
live globally. I'll change the patch accordingly.

> the launcher couldn't start a worker because the max numbers of workers was

Good idea.

Regards,

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 23 Feb 2022 03:00:08 +0000

> temporarily chose pg_stat_subscription_activity.

Thanks for your patch.

Few comments:

1.
+       <structfield>apply_error_count</structfield> <type>uint8</type>
...
+       <structfield>sync_error_count</structfield> <type>uint8</type>

It seems that Postgres has no data type named uint8, should we change it to
bigint?

2.
+# Wait for the table sync error to be reported.
+$node_subscriber->poll_query_until(
+	'postgres',
+	qq[
+SELECT apply_error_count = 0 AND sync_error_count > 0
+FROM pg_stat_subscription_activity
+WHERE subname = 'tap_sub'
+]) or die "Timed out while waiting for table sync error";

We want to check table sync error here, but do we need to check
"apply_error_count = 0"? I am not sure if it is possible that the apply worker has
an unexpected error, which would cause this test to fail.

Regards,
Tang

---

From: Peter Smith <[email-masked]>
Date: Wed, 23 Feb 2022 14:07:45 +1100

Hi. Below are my review comments for the v1 patch.

======

1. Commit message - wording

SUGGESTION
It was decided (refer to the Discussion link below) that the stats
collector is not an appropriate place to store the error information of
subscription workers.

~~~

2. Commit message - wording

SUGGESTION
"This commits changes the view" --> "This patch changes the
pg_stat_subscription_workers view"

~~~

3. Commit message - wording

Removing these error details, since we don't need to record the
error information for apply workers and tablesync workers separately,
the view now has one entry per subscription.

DID THIS MEAN...
After removing these error details, there is no longer separate
information for apply workers and tablesync workers, so the view now
has only one entry per subscription.

--

But anyway, that is not entirely true either because those counters
are separate information for the different workers, right?

~~

4. Commit message - wording

Also, it changes the view name to pg_stat_subscription_activity
since the word "worker" is an implementation detail that we use one
worker for one tablesync.

SUGGESTION
"Also, it changes" --> "The patch also changes" ...

~~~

5. doc/src/sgml/monitoring.sgml - wording

   <para>
-   The <structname>pg_stat_subscription_workers</structname> view will contain
-   one row per subscription worker on which errors have occurred, for workers
-   applying logical replication changes and workers handling the initial data
-   copy of the subscribed tables.  The statistics entry is removed when the
+   The <structname>pg_stat_subscription_activity</structname> view will contain
+   one row per subscription.  The statistics entry is removed when the
    corresponding subscription is dropped.
   </para>

SUGGESTION
"The statistics entry is removed" --> "This row is removed"

~~~

6.  doc/src/sgml/monitoring.sgml - why two counters?

Please forgive this noob question...

I see there are 2 error_count columns (one for each kind of worker)
but I was wondering why it is useful for users to be able to
distinguish if the error came from the tablesync workers or from the
apply workers? Do you have any example?

Also, IIRC sometimes the tablesync might actually do a few "apply"
changes itself... so the distinction may become a bit fuzzy...

~~~

7. src/backend/postmaster/pgstat.c - comment

@@ -1313,13 +1312,13 @@ pgstat_vacuum_stat(void)
  }

  /*
- * Repeat for subscription workers.  Similarly, we needn't bother in the
- * common case where no subscription workers' stats are being collected.
+ * Repeat for subscription.  Similarly, we needn't bother in the common
+ * case where no subscription stats are being collected.
  */

typo?

"Repeat for subscription." --> "Repeat for subscriptions."

~~~

8. src/backend/postmaster/pgstat.c

@@ -3000,32 +2968,29 @@ pgstat_fetch_stat_funcentry(Oid func_id)

 /*
  * ---------
- * pgstat_fetch_stat_subworker_entry() -
+ * pgstat_fetch_stat_subentry() -
  *
  * Support function for the SQL-callable pgstat* functions. Returns
- * the collected statistics for subscription worker or NULL.
+ * the collected statistics for subscription or NULL.
  * ---------
  */
-PgStat_StatSubWorkerEntry *
-pgstat_fetch_stat_subworker_entry(Oid subid, Oid subrelid)
+PgStat_StatSubEntry *
+pgstat_fetch_stat_subentry(Oid subid)

There seems some kind of inconsistency because the member name is
called "subscriptions" but sometimes it seems singular.

Some places (e.g. pgstat_vacuum_stat) will iterate multiple results,
but then other places (like this function) just return to a single
"subscription" (or "entry").

I suspect all the code may be fine; probably it is just some
inconsistent (singular/plural) comments that have confused things a
bit.

~~~

9. src/backend/replication/logical/worker.c - subscription id

+ /* Report the worker failed during table synchronization */
+ pgstat_report_subscription_error(MyLogicalRepWorker->subid, false);

and

+ /* Report the worker failed during the application of the change */
+ pgstat_report_subscription_error(MyLogicalRepWorker->subid, true);

Why don't these use MySubscription->oid instead of MyLogicalRepWorker->subid?

~~~

10. src/include/pgstat.h - enum order

@@ -84,8 +84,8 @@ typedef enum StatMsgType
  PGSTAT_MTYPE_REPLSLOT,
  PGSTAT_MTYPE_CONNECT,
  PGSTAT_MTYPE_DISCONNECT,
+ PGSTAT_MTYPE_SUBSCRIPTIONERROR,
  PGSTAT_MTYPE_SUBSCRIPTIONPURGE,
- PGSTAT_MTYPE_SUBWORKERERROR,
 } StatMsgType;

This change rearranges the enum order. Maybe it is safer not to do this?

~~~

11. src/include/pgstat.h

@@ -767,8 +747,8 @@ typedef union PgStat_Msg
  PgStat_MsgReplSlot msg_replslot;
  PgStat_MsgConnect msg_connect;
  PgStat_MsgDisconnect msg_disconnect;
+ PgStat_MsgSubscriptionError msg_subscriptionerror;
  PgStat_MsgSubscriptionPurge msg_subscriptionpurge;
- PgStat_MsgSubWorkerError msg_subworkererror;
 } PgStat_Msg;

This change also rearranges the order. Maybe there was no good reason
to do that?

~~~

12. src/include/pgstat.h - PgStat_StatDBEntry

@@ -823,16 +803,12 @@ typedef struct PgStat_StatDBEntry
  TimestampTz stats_timestamp; /* time of db stats file update */

  /*
- * tables, functions, and subscription workers must be last in the struct,
- * because we don't write the pointers out to the stats file.
- *
- * subworkers is the hash table of PgStat_StatSubWorkerEntry which stores
- * statistics of logical replication workers: apply worker and table sync
- * worker.
+ * tables, functions, and subscription must be last in the struct, because
+ * we don't write the pointers out to the stats file.
  */

Should that say "tables, functions, and subscriptions" (plural)

------

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Wed, 23 Feb 2022 09:17:31 +0530

On Tue, Feb 22, 2022 at 8:02 PM Masahiko Sawada <[email-masked]> wrote:
> overhead in practice but I'd like to avoid it.
>

I am not sure about overhead but I see another problem if we use that
approach. In the exit path, logicalrep_worker_onexit() will get called
before pgstat_report_stat() and that will clear the
MyLogicalRepWorker->subid, so we won't know the id for which to send
stats. So, the way patch is doing seems reasonable to me unless
someone has better ideas.

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Wed, 23 Feb 2022 17:20:58 +1100

Below are my review comments just for the v1 patch test code.

======

1. "table sync error" versus "tablesync error"

+
+
 # Wait for initial table sync for test_tab1 to finish.

IMO all these "table sync" should be changed to "tablesync", because a
table "sync error" sounds like something completely different to a
"tablesync error".

SUGGESTIONS
- "Wait for the table sync error to be reported." --> "Wait for the
tablesync error to be reported."
- "Timed out while waiting for table sync error" --> "Timed out while
waiting for tablesync error"
- "Truncate test_tab1 so that table sync can continue." -->  "Truncate
test_tab1 so that tablesync worker can fun to completion."
- "Wait for initial table sync for test_tab1 to finish." --> "Wait for
the tablesync worker of test_tab1 to finish."

~~~

2. Unnecessary INSERT VALUES (2)?

(I think this is a duplicate of what [Osumi] #6 reported)

+# 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)");

Why does the test do INSERT data (2)? There is already data (1) from
the tablesync which will cause an apply worker PK violation when
another VALUES (1) is published.

Note, the test comment also needs to change...

~~~

3. Wait for the apply worker error

+# Wait for the apply error to be reported.
+$node_subscriber->poll_query_until(
+ 'postgres',
+ qq[
+SELECT apply_error_count > 0 AND sync_error_count > 0
+FROM pg_stat_subscription_activity
+WHERE subname = 'tap_sub'
+]) or die "Timed out while waiting for apply error";

This test is only for apply worker errors. So why is the test SQL
checking "AND sync_error_count > 0"?

(This is similar to what [Tang] #2 reported, but I think she was
referring to the other tablesync test)

~~~

4. Wrong worker?

(looks like a duplicate of what [Osumi] #5 already)

+

 $node_subscriber->stop('fast');
 $node_publisher->stop('fast');

Cut/paste error? Aren't you doing TRUNCATE here so the apply worker
can continue; not the tablesync worker (which already completed)

"Truncate test_tab1 so that table sync can continue." --> "Truncate
test_tab1 so that the apply worker can continue."

------
[Osumi] https://www.postgresql.org/message-id/CAD21AoBRt%3DcyKsZP83rcMkHnT498gHH0TEP34fZBrGCxT-Ahwg%40mail.gmail.com
[Tang] https://www.postgresql.org/message-id/TYCPR01MB612840D018FEBD38268CC83BFB3C9%40TYCPR01MB6128.jpnprd01.prod.outlook.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 24 Feb 2022 10:32:54 +0900

Hi,

> Hi. Below are my review comments for the v1 patch.

Thank you for the comments! I've attached the latest version patch
that incorporated all comments I got so far. The primary change from
the previous version is that the subscription statistics live globally
rather than per-database.

> collector is not an appropriate place to store the error information of

Fixed.

> pg_stat_subscription_workers view"

Fixed.

> has only one entry per subscription.

Right. Since I also made the subscription statistics a cluster-wide
statistics, I've changed this part accordingly.

> "Also, it changes" --> "The patch also changes" ...

Fixed.

> "The statistics entry is removed" --> "This row is removed"

On second thoughts, this sentence is not necessary since it's obvious
and descriptions of other stats view don't mention it.

> changes itself... so the distinction may become a bit fuzzy...

I think that the tablesync phase and the apply phase can fail for
different reasons. So these values would be a good indicator for users
to check if each phase works fine.

After more thoughts, I think it's better to increment sync_error_count
also when a tablesync worker fails while applying the changes. These
counters will correspond to the error information entries that will be
stored in a system catalog.

> "Repeat for subscription." --> "Repeat for subscriptions."

Fixed.

> @@ -3000,32 +2968,29 @@ pgstat_fetch_stat_funcentry(Oid func_id)

Fixed.

> Why don't these use MySubscription->oid instead of MyLogicalRepWorker->subid?

It's just because we used to use MyLogicalRepWorker->subid, is there
any particular reason why we should use MySubscription->oid here?

> This change rearranges the enum order. Maybe it is safer not to do this?

I assume you're concerned about binary compatibility or something. I
think it should not be a problem since both
PGSTAT_MTYPE_SUBWORKERERROR and PGSTAT_MTYPE_SUBSCRIPTIONPURGE are
introduced to PG15.

> This change also rearranges the order. Maybe there was no good reason

It's for keeping the alphabetical order within subscription-related messages.

> Should that say "tables, functions, and subscriptions" (plural)

This part is removed in the latest patch.

> It seems that Postgres has no data type named uint8, should we change it to

Right, fixed.

> an unexpected error, which would cause this test to fail.

Yeah, it seems better not to have this condition, fixed.

> the tablesync worker of test_tab1 to finish."

Fixed.

> referring to the other tablesync test)

Fixed.

Regards,

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 24 Feb 2022 07:20:49 +0000

> rather than per-database.

Thanks for updating the patch.

Few comments:

1. 
I think we should add some doc for column stats_reset in pg_stat_subscription_activity view.

2.
+CREATE VIEW pg_stat_subscription_activity AS
     SELECT
-        w.subid,
+        a.subid,
         s.subname,
...
+        a.apply_error_count,
+        a.sync_error_count,
+	a.stats_reset
+    FROM pg_subscription as s,
+        pg_stat_get_subscription_activity(oid) as a;

The line "a.stats_reset" uses a Tab, and we'd better use spaces here.

Regards,
Tang

---

From: Peter Smith <[email-masked]>
Date: Thu, 24 Feb 2022 19:53:33 +1100

Hi. Below are my review comments for the v2 patch.

======

1. Commit message

This patch changes the pg_stat_subscription_workers view (introduced
by commit 8d74fc9) so that it stores only statistics counters:
apply_error_count and sync_error_count, and has one entry for
subscription.

SUGGESTION
"and has one entry for subscription." --> "and has one entry for each
subscription."

~~~

2. Commit message

After removing these error details, there are no longer relation
information, so the subscription statistics are now a cluster-wide
statistics.

SUGGESTION
"there are no longer relation information," --> "there is no longer
any relation information,"

~~~

3. doc/src/sgml/monitoring.sgml

-      <para>
-       The error message
+       Number of times the error occurred during the application of changes
       </para></entry>
      </row>

      <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>bigint</type>
       </para>
       <para>
-       Last time at which this error occurred
+       Number of times the error occurred during the initial table
+       synchronization
       </para></entry>

SUGGESTION (both places)
"Number of times the error occurred ..." --> "Number of times an error
occurred ..."

~~~

4. doc/src/sgml/monitoring.sgml - missing column

(duplicate - also reported by [Tang-v2])

The PG docs for the new "stats_reset" column are missing.

~~~

5. src/backend/catalog/system_views.sql - whitespace

(duplicate - also reported by [Tang-v2])

-          JOIN pg_subscription s ON (w.subid = s.oid);
+        a.apply_error_count,
+        a.sync_error_count,
+ a.stats_reset
+    FROM pg_subscription as s,
+        pg_stat_get_subscription_activity(oid) as a;

inconsistent tab/space indenting for 'a.stats_reset'.

~~~

6. src/backend/postmaster/pgstat.c - function name

+/* ----------
+ * pgstat_reset_subscription_counter() -
+ *
+ * Tell the statistics collector to reset a single subscription
+ * counter, or all subscription counters (when subid is InvalidOid).
+ *
+ * Permission checking for this function is managed through the normal
+ * GRANT system.
+ * ----------
+ */
+void
+pgstat_reset_subscription_counter(Oid subid)

SUGGESTION (function name)
"pgstat_reset_subscription_counter" -->
"pgstat_reset_subscription_counters" (plural)

~~

7. src/backend/postmaster/pgstat.c - pgstat_recv_resetsubcounter

@@ -5645,6 +5598,51 @@
pgstat_recv_resetreplslotcounter(PgStat_MsgResetreplslotcounter *msg,
  }
 }

+/* ----------
+ * pgstat_recv_resetsubcounter() -
+ *
+ * Reset some subscription statistics of the cluster.
+ * ----------
+ */
+static void
+pgstat_recv_resetsubcounter(PgStat_MsgResetsubcounter *msg, int len)

"Reset some" seems a bit vague. Why not describe that it is all or
none according to the msg->m_subid?

~~~

8. src/backend/postmaster/pgstat.c - pgstat_recv_resetsubcounter

+ if (!OidIsValid(msg->m_subid))
+ {
+ HASH_SEQ_STATUS sstat;
+
+ /* Clear all subscription counters */
+ hash_seq_init(&sstat, subscriptionStatHash);
+ while ((subentry = (PgStat_StatSubEntry *) hash_seq_search(&sstat)) != NULL)
+ pgstat_reset_subscription(subentry, ts);
+ }
+ else
+ {
+ /* Get the subscription statistics to reset */
+ subentry = pgstat_get_subscription_entry(msg->m_subid, false);
+
+ /*
+ * Nothing to do if the given subscription entry is not found.  This
+ * could happen when the subscription with the subid is removed and
+ * the corresponding statistics entry is also removed before receiving
+ * the reset message.
+ */
+ if (!subentry)
+ return;
+
+ /* Reset the stats for the requested replication slot */
+ pgstat_reset_subscription(subentry, ts);
+ }
+}

Why not reverse the if/else?

Checking OidIsValid(...) seems more natural than checking !OidIsValid(...)

~~~

9. src/backend/postmaster/pgstat.c - pgstat_recv_subscription_purge

static void
pgstat_recv_subscription_purge(PgStat_MsgSubscriptionPurge *msg, int len)
{
/* Return if we don't have replication subscription statistics */
if (subscriptionStatHash == NULL)
return;

/* Remove from hashtable if present; we don't care if it's not */
(void) hash_search(subscriptionStatHash, (void *) &(msg->m_subid),
   HASH_REMOVE, NULL);
}

SUGGESTION
Wouldn't the above code be simpler written like:

if (subscriptionStatHash)
{
/* Remove from hashtable if present; we don't care if it's not */
(void) hash_search(subscriptionStatHash, (void *) &(msg->m_subid),
   HASH_REMOVE, NULL);
}
~~~

10. src/backend/replication/logical/worker.c

(from my previous [Peter-v1] #9)

> any particular reason why we should use MySubscription->oid here?

I felt MySubscription->oid is a more natural and more direct way of
expressing the same thing.

Consider:  "the oid of the current subscription" versus "the oid of
the subscription of the current worker". IMO the first one is simpler.
YMMV.

Also, it is shorter :)

~~~

11. src/include/pgstat.h  - enum order

(follow-on from my previous v1 review comment #10)

>PGSTAT_MTYPE_SUBWORKERERROR and PGSTAT_MTYPE_SUBSCRIPTIONPURGE are

Yes, maybe it is OK for those ones. But now in v2 there is a new
PGSTAT_MTYPE_RESETSUBCOUNTER.

Shouldn't at least that one be put at the end for the same reason?

~~~

12. src/include/pgstat.h  - PgStat_MsgResetsubcounter

Maybe a better name for this is "PgStat_MsgResetsubcounters" (plural)?

~~~

13. src/test/subscription/t/026_worker_stats.pl - missing test?

Shouldn't there also be some test to reset the counters to confirm
that they really do get reset to zero?

~~~

14. src/tools/pgindent/typedefs.list

PgStat_MsgResetsubcounter (from pgstat.h) is missing?

------

15. pg_stat_subscription_activity view name?

Has the view name already been decided or still under discussion - I
was not sure?

If is it already decided then fine, but if not then my vote would be
for something different like:
e.g.1 - pg_stat_subscription_errors
e.g.2 - pg_stat_subscription_counters
e.g.3 - pg_stat_subscription_metrics

Maybe "activity" was chosen to be deliberately vague in case some
future unknown stats columns get added? But it means now there is a
corresponding function "pg_stat_reset_subscription_activity", when in
practice you don't really reset activity - what you want to do is
reset some statistics *about* the activity... so it all seems a bit
odd to me.

------
[Tang-v2] https://www.postgresql.org/message-id/OS0PR01MB6113769B17E90ADC9ACA14B2FB3D9%40OS0PR01MB6113.jpnprd01.prod.outlook.com
[Peter-v1] https://www.postgresql.org/message-id/CAHut%2BPtH-uN5rbGRh-%3DkCd8xvQYDf_JCcjLcVjW3OXGz6T%2BxCw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 24 Feb 2022 10:20:39 +0100

> It was a joke, making light of our bad naming in pg_stat_*, not a serious

I think pg_stat_subscription_stats is actually the least worst option.

Unless we want to consider renaming pg_stat_subscription (which is 
actually more like pg_stat_subscription_activity).  But I think that 
should be avoided.

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 24 Feb 2022 10:48:41 +0100

> global catalog, so it seems to make more sense to have them "live" globally as

pg_subscription being a global catalog is a bit of a lie for the benefit 
of the worker launcher, but it can be treated as a per-database catalog 
for practical purposes.

> the launcher couldn't start a worker because the max numbers of workers was

That's a reasonable point, however.

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 24 Feb 2022 10:53:02 +0100

> rather than per-database.

I don't think the name pg_stat_subscription_activity is a good choice.

We have a view called pg_stat_activity, which is very well known.  From 
that perspective, "activity" means what is happening right now or what 
has happened most recently.  The reworked view in this patch does not 
contain that (we already have pg_stat_subscription for that), but it 
contains accumulated counters.

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 24 Feb 2022 20:46:04 +0900

> contains accumulated counters.

Right.

What pg_stat_subscription shows is rather suitable for the name
pg_stat_subscription_activity than the reworked view. But switching
these names would also not be a good idea.  I think it's better to use
"subscription" in the view name since it shows actually statistics for
subscriptions and subscription OID is the key. I personally prefer
pg_stat_subscription_counters among the ideas that have been proposed
so far, but I'd like to hear opinions and votes.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Thu, 24 Feb 2022 17:35:26 +0530

> (void) hash_search(subscriptionStatHash, (void *) &(msg->m_subid),

I think we can write that way as well but I would prefer the way it is
currently in the patch as we use a similar pattern in nearby code (ex.
pgstat_recv_resetreplslotcounter) and at other places in the code base
as well.

> the subscription of the current worker". IMO the first one is simpler.

I think we can use either but maybe MySubscription->oid would be
slightly better here as the same is used in nearby code as well.

> Shouldn't at least that one be put at the end for the same reason?

I don't see the reason to put that at end. It is better to add it near
to similar RESET enums.

> that they really do get reset to zero?

I think we avoid writing tests for stats for each and every case as it
is not reliable in nature (the message can be lost). If we can find a
reliable way then it is okay.

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 24 Feb 2022 21:17:55 +0900

> slightly better here as the same is used in nearby code as well.

Okay, will change.

> reliable way then it is okay.

Yeah, the messages can even be out-of-order. Particularly, in this
test, the apply worker and table sync worker keep reporting the
messages, it's quite possible that the test becomes unstable. I
remember we removed unstable tests of resetting statistics before
(e.g., see fc6950913).

Regards,

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 24 Feb 2022 13:23:55 +0100

> so far, but I'd like to hear opinions and votes.

_counters will fail if there is something not a counter (such as 
last-timestamp-of-something).

Earlier, pg_stat_subscription_stats was mentioned, which doesn't have 
that problem.

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 24 Feb 2022 21:51:05 +0900

> Earlier, pg_stat_subscription_stats was mentioned, which doesn't have

Ah, I had misunderstood your comment. Right, _counter could be a
blocker for the future changes.

Regards,

---

From: Andres Freund <[email-masked]>
Date: Thu, 24 Feb 2022 06:45:38 -0800

Hi,

On 2022-02-24 13:23:55 +0100, Peter Eisentraut wrote:
> Earlier, pg_stat_subscription_stats was mentioned, which doesn't have that
> problem.

We really should try to fix this in a more general way at some point. We have
way too many different things mixed up in pg_stat_*.

I'd like to get something like the patch in soon though, we can still change
the name later. I've been blocked behind this stuff for weeks, and it's
getting really painful...

Greetings,

Andres Freund

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 25 Feb 2022 00:49:37 +0900

Hi,

> I think we should add some doc for column stats_reset in pg_stat_subscription_activity view.

Added.

Fixed.

Fixed.

Fixed.

Fixed.

Fixed.

> 7. src/backend/postmaster/pgstat.c - pgstat_recv_resetsubcounter

I think it reset none, one, or all statistics, actually. Given other
pgstat_recv_reset* functions also have similar comments, I think we
can use it rather than elaborating.

> Checking OidIsValid(...) seems more natural than checking !OidIsValid(...)

Yes, but it's because we use the same pattern in the near function
(see pgstat_recv_resetreplslotcounter()).

> (void) hash_search(subscriptionStatHash, (void *) &(msg->m_subid),

Similarly, as Amit also mentioned, there is a similar pattern in the
near function. So keep it as it is

> the subscription of the current worker". IMO the first one is simpler.

Changed.

> Shouldn't at least that one be put at the end for the same reason?

I think it's better to put it near similar RESET enums.

> Maybe a better name for this is "PgStat_MsgResetsubcounters" (plural)?

I think it's better to be consistent with other similar message
structs (e.g., msg_resetsharedcounter and msg_resetslrucounter).

> PgStat_MsgResetsubcounter (from pgstat.h) is missing?

Added.

> reset some statistics *about* the activity... so it all seems a bit

Yes, it still needs discussion.

I've attached the updated patch.

Regards,

---

From: Peter Smith <[email-masked]>
Date: Fri, 25 Feb 2022 12:56:04 +1100

Below are my review comments for the v3 patch.

======

1. Commit message

(An earlier review comment [Peter-v2] #2 was only partly fixed)

"there are no longer any relation information" --> "there is no longer
any relation information"

~~~

2. doc/src/sgml/monitoring.sgml

+      <entry><structname>pg_stat_subscription_activity</structname><indexterm><primary>pg_stat_subscription_activity</primary></indexterm></entry>
+      <entry>One row per subscription, showing statistics about subscription
+      activity.
+      See <link linkend="monitoring-pg-stat-subscription-activity">
+      <structname>pg_stat_subscription_activity</structname></link>
for details.
       </entry>
      </row>

Currently these stats are only about errors. These are not really
statistics about "activity" though. Probably it is better just to
avoid that word altogether?

SUGGESTIONS

e.g.1. "One row per subscription, showing statistics about
subscription activity." --> "One row per subscription, showing
statistics about errors."

e.g.2. "One row per subscription, showing statistics about
subscription activity." --> "One row per subscription, showing
statistics about that subscription."

-----
[Peter-v2] https://www.postgresql.org/message-id/CAHut%2BPv%3DVmXtHmPKp4fg8VDF%2BTQP6xWgL91Jn-hrqg5QObfCZA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Fri, 25 Feb 2022 10:56:47 +0530

> > "pgstat_reset_subscription_counters" (plural)

We don't use the plural form in other similar cases like
pgstat_reset_replslot_counter, pgstat_reset_slru_counter, so why do it
differently here?

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Sat, 26 Feb 2022 08:21:23 +0530

On Thu, Feb 24, 2022 at 9:20 PM Masahiko Sawada <[email-masked]> wrote:

I have reviewed the latest version and made a few changes along with
fixing some of the pending comments by Peter Smith. The changes are as
follows: (a) Removed m_databaseid in PgStat_MsgSubscriptionError as
that is not required now; (b) changed the struct name
PgStat_MsgSubscriptionPurge to PgStat_MsgSubscriptionDrop to make it
similar to DropDb; (c) changed the view name to
pg_stat_subscription_stats, we can reconsider it in future if there is
a consensus on some other name, accordingly changed the reset function
name to pg_stat_reset_subscription_stats; (d) moved some of the newly
added subscription stats functions adjacent to slots to main the
consistency in code; (e) changed comments at few places; (f) added
LATERAL back to system_views query as we refer pg_subscription's oid
in the function call, previously that was not clear.

Do let me know what you think of the attached?

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Sat, 26 Feb 2022 08:24:07 +0530

...
> statistics about errors."

I preferred this one and made another change suggested by you in the
latest version posted by me. Thanks!

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 28 Feb 2022 10:18:33 +0900

On Sat, Feb 26, 2022 at 11:51 AM Amit Kapila <[email-masked]> wrote:
> fixing some of the pending comments by Peter Smith.

Thank you for updating the patch!

> The changes are as
> consistency in code; (e) changed comments at few places;

Agreed.

> (f) added

I think LATERAL is still unnecessary as you pointed out before. The
documentation[1] says,

LATERAL can also precede a function-call FROM item, but in this case
it is a noise word, because the function expression can refer to
earlier FROM items in any case.

The rest looks good to me.

Regards,

[1] https://www.postgresql.org/docs/devel/sql-select.html

---

From: Peter Smith <[email-masked]>
Date: Mon, 28 Feb 2022 14:29:13 +1100

Below are my comments for the v4 patch.

These are only nitpicking comments now; Otherwise, it LGTM.

(Sorry, now I see there are some overlaps with comments posted in the
last 20 mins so take or leave these as you wish)

======

1. doc/src/sgml/monitoring.sgml

-      <para>
-       OID of the relation that the worker was processing when the
-       error occurred
+       Number of times an error occurred during the application of changes
       </para></entry>
      </row>

BEFORE
Number of times an error occurred during the application of changes
SUGGESTED
Number of times an error occurred while applying changes

~~~

+        Resets statistics for a single subscription shown in the
+        <structname>pg_stat_subscription_stats</structname> view to zero. If
+        the argument is <literal>NULL</literal>, reset statistics for all
+        subscriptions.
        </para>

SUGGESTED (simpler description, more similar to pg_stat_reset_replication_slot)
Reset statistics to zero for a single subscription. If the argument is
<literal>NULL</literal>, reset statistics for all subscriptions.

~~~

3. src/backend/replication/logical/worker.c - comment

+ /*  */
+ pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_worker());

BEFORE
Report the worker failed during the application of the change
SUGGESTED
Report the worker failed while applying changes

~~~

4. src/include/pgstat.h - comment

+typedef struct PgStat_MsgResetsubcounter
+{
+ PgStat_MsgHdr m_hdr;
+ Oid m_subid; /* InvalidOid for clearing all subscription
+ * stats */
+} PgStat_MsgResetsubcounter;

BEFORE
InvalidOid for clearing all subscription stats
SUGGESTED
InvalidOid means reset all subscription stats

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Mon, 28 Feb 2022 09:20:11 +0530

> <literal>NULL</literal>, reset statistics for all subscriptions.

As discussed, it is better to keep the view name in this description
important as we have another view (pg_stat_susbcription) as well. So,
I am planning to retain the current wording.

-- 
With Regards,
Amit Kapila.