Re: Issue with pg_stat_subscription_stats
On Tue, Jul 12, 2022 at 09:31:16AM +0530, Amit Kapila wrote: > I am not against backpatching this but OTOH it doesn't appear critical > enough to block one's work, so not backpatching should be fine. We are just talking about the reset timestamp not being set at when the object is created, right? This does not strike me as critical, so applying it only on HEAD is fine IMO. A few months ago, while in beta, I would have been fine with something applied to REL_15_STABLE. Now that we are in RC, that's not worth taking a risk in my opinion. Amit or Andres, are you planning to double-check and perhaps merge this patch to take care of the inconsistency? -- Michael signature.asc Description: PGP signature
Re: Issue with pg_stat_subscription_stats
Hi, On 2022-10-06 14:10:56 +0900, Michael Paquier wrote: > On Tue, Jul 12, 2022 at 09:31:16AM +0530, Amit Kapila wrote: > > I am not against backpatching this but OTOH it doesn't appear critical > > enough to block one's work, so not backpatching should be fine. > > We are just talking about the reset timestamp not being set at > when the object is created, right? This does not strike me as > critical, so applying it only on HEAD is fine IMO. A few months ago, > while in beta, I would have been fine with something applied to > REL_15_STABLE. Now that we are in RC, that's not worth taking a risk > in my opinion. Agreed. > Amit or Andres, are you planning to double-check and perhaps merge > this patch to take care of the inconsistency? I'll run it through CI and then to master unless somebody pipes up in the meantime. Thanks for bringing this thread up, I'd lost track of it. Greetings, Andres Freund
Re: Issue with pg_stat_subscription_stats
On 2022-10-06 16:43:43 -0700, Andres Freund wrote: > On 2022-10-06 14:10:56 +0900, Michael Paquier wrote: > > On Tue, Jul 12, 2022 at 09:31:16AM +0530, Amit Kapila wrote: > > > I am not against backpatching this but OTOH it doesn't appear critical > > > enough to block one's work, so not backpatching should be fine. > > > > We are just talking about the reset timestamp not being set at > > when the object is created, right? This does not strike me as > > critical, so applying it only on HEAD is fine IMO. A few months ago, > > while in beta, I would have been fine with something applied to > > REL_15_STABLE. Now that we are in RC, that's not worth taking a risk > > in my opinion. > > Agreed. > > > Amit or Andres, are you planning to double-check and perhaps merge > > this patch to take care of the inconsistency? > > I'll run it through CI and then to master unless somebody pipes up in the > meantime. And pushed. Thanks all!
Re: Issue with pg_stat_subscription_stats
On Fri, Oct 7, 2022 at 9:27 AM Andres Freund wrote: > > On 2022-10-06 16:43:43 -0700, Andres Freund wrote: > > On 2022-10-06 14:10:56 +0900, Michael Paquier wrote: > > > On Tue, Jul 12, 2022 at 09:31:16AM +0530, Amit Kapila wrote: > > > > I am not against backpatching this but OTOH it doesn't appear critical > > > > enough to block one's work, so not backpatching should be fine. > > > > > > We are just talking about the reset timestamp not being set at > > > when the object is created, right? This does not strike me as > > > critical, so applying it only on HEAD is fine IMO. A few months ago, > > > while in beta, I would have been fine with something applied to > > > REL_15_STABLE. Now that we are in RC, that's not worth taking a risk > > > in my opinion. > > > > Agreed. > > > > > Amit or Andres, are you planning to double-check and perhaps merge > > > this patch to take care of the inconsistency? > > > > I'll run it through CI and then to master unless somebody pipes up in the > > meantime. > > And pushed. Thanks all! Thanks! Regards, -- Masahiko Sawada PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Issue with pg_stat_subscription_stats
On Thu, Oct 06, 2022 at 04:43:43PM -0700, Andres Freund wrote: > Thanks for bringing this thread up, I'd lost track of it. The merit goes to Sawada-san here, who has poked me about this thread :p -- Michael signature.asc Description: PGP signature
Re: Issue with pg_stat_subscription_stats
Hi, On 2022-07-04 11:01:01 +0900, Masahiko Sawada wrote: > I've attached the patch, fix_drop_subscriptions_stats.patch, to fix it. LGTM. Unless somebody sees a reason not to, I'm planning to commit that to 15 and HEAD. > I've also attached another PoC patch, > poc_create_subscription_stats.patch, to create the stats entry when > creating the subscription, which address the issue reported in this > thread; the pg_stat_reset_subscription_stats() doesn't update the > stats_reset if no error is reported yet. It'd be good for this to include a test. > diff --git a/src/backend/utils/activity/pgstat_subscription.c > b/src/backend/utils/activity/pgstat_subscription.c > index e1072bd5ba..ef318b7422 100644 > --- a/src/backend/utils/activity/pgstat_subscription.c > +++ b/src/backend/utils/activity/pgstat_subscription.c > @@ -47,8 +47,20 @@ pgstat_report_subscription_error(Oid subid, bool > is_apply_error) > void > pgstat_create_subscription(Oid subid) > { > + PgStat_EntryRef *entry_ref; > + PgStatShared_Subscription *shstatent; > + > pgstat_create_transactional(PGSTAT_KIND_SUBSCRIPTION, > InvalidOid, > subid); > + > + entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_SUBSCRIPTION, > + > InvalidOid, subid, > + > false); > + shstatent = (PgStatShared_Subscription *) entry_ref->shared_stats; > + > + memset(&shstatent->stats, 0, sizeof(shstatent->stats)); > + > + pgstat_unlock_entry(entry_ref); > } > > /* I think most of this could just be pgstat_reset_entry(). Greetings, Andres Freund
Re: Issue with pg_stat_subscription_stats
On Wed, Jul 6, 2022 at 6:52 AM Andres Freund wrote: > > Hi, > > On 2022-07-04 11:01:01 +0900, Masahiko Sawada wrote: > > I've attached the patch, fix_drop_subscriptions_stats.patch, to fix it. > > LGTM. Unless somebody sees a reason not to, I'm planning to commit that to 15 > and HEAD. > > > > I've also attached another PoC patch, > > poc_create_subscription_stats.patch, to create the stats entry when > > creating the subscription, which address the issue reported in this > > thread; the pg_stat_reset_subscription_stats() doesn't update the > > stats_reset if no error is reported yet. > > It'd be good for this to include a test. Agreed. > > > > diff --git a/src/backend/utils/activity/pgstat_subscription.c > > b/src/backend/utils/activity/pgstat_subscription.c > > index e1072bd5ba..ef318b7422 100644 > > --- a/src/backend/utils/activity/pgstat_subscription.c > > +++ b/src/backend/utils/activity/pgstat_subscription.c > > @@ -47,8 +47,20 @@ pgstat_report_subscription_error(Oid subid, bool > > is_apply_error) > > void > > pgstat_create_subscription(Oid subid) > > { > > + PgStat_EntryRef *entry_ref; > > + PgStatShared_Subscription *shstatent; > > + > > pgstat_create_transactional(PGSTAT_KIND_SUBSCRIPTION, > > InvalidOid, > > subid); > > + > > + entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_SUBSCRIPTION, > > + > > InvalidOid, subid, > > + > > false); > > + shstatent = (PgStatShared_Subscription *) entry_ref->shared_stats; > > + > > + memset(&shstatent->stats, 0, sizeof(shstatent->stats)); > > + > > + pgstat_unlock_entry(entry_ref); > > } > > > > /* > > I think most of this could just be pgstat_reset_entry(). I think pgstat_reset_entry() doesn't work for this case as it skips resetting the entry if it doesn't exist. I've attached an updated patch. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ 0001-Create-subscription-stats-entry-on-CREATE-SUBSCRIPTI.patch Description: Binary data
Re: Issue with pg_stat_subscription_stats
On 2022-07-06 10:25:02 +0900, Masahiko Sawada wrote: > > I think most of this could just be pgstat_reset_entry(). > > I think pgstat_reset_entry() doesn't work for this case as it skips > resetting the entry if it doesn't exist. True - but a pgstat_get_entry_ref(create = true); pgstat_reset_entry(); would still be shorter?
Re: Issue with pg_stat_subscription_stats
On Wed, Jul 6, 2022 at 10:48 AM Andres Freund wrote: > > On 2022-07-06 10:25:02 +0900, Masahiko Sawada wrote: > > > I think most of this could just be pgstat_reset_entry(). > > > > I think pgstat_reset_entry() doesn't work for this case as it skips > > resetting the entry if it doesn't exist. > > True - but a pgstat_get_entry_ref(create = true); pgstat_reset_entry(); would > still be shorter? Indeed. I've updated the patch. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ v2-0001-Create-subscription-stats-entry-at-CREATE-SUBSCRI.patch Description: Binary data
Re: Issue with pg_stat_subscription_stats
Hi, On 2022-07-06 11:41:46 +0900, Masahiko Sawada wrote: > diff --git a/src/test/regress/sql/subscription.sql > b/src/test/regress/sql/subscription.sql > index 74c38ead5d..6a46956f6e 100644 > --- a/src/test/regress/sql/subscription.sql > +++ b/src/test/regress/sql/subscription.sql > @@ -30,6 +30,12 @@ CREATE SUBSCRIPTION regress_testsub CONNECTION > 'dbname=regress_doesnotexist' PUB > COMMENT ON SUBSCRIPTION regress_testsub IS 'test subscription'; > SELECT obj_description(s.oid, 'pg_subscription') FROM pg_subscription s; > > +-- Check if the subscription stats are created and stats_reset is updated > +-- by pg_stat_reset_subscription_stats(). > +SELECT subname, stats_reset IS NULL stats_reset_is_null FROM > pg_stat_subscription_stats ORDER BY 1; Why use ORDER BY 1 instead of just getting the stats for the subscription we want to test? Seems a bit more robust to show only that one, so we don't get unnecessary changes if the test needs to create another subscription or such. > +SELECT pg_stat_reset_subscription_stats(oid) FROM pg_subscription; > +SELECT subname, stats_reset IS NULL stats_reset_is_null FROM > pg_stat_subscription_stats ORDER BY 1; > + Perhaps worth resetting again and checking that the timestamp is bigger than the previous timestamp? You can do that with \gset etc. Greetings, Andres Freund
Re: Issue with pg_stat_subscription_stats
On 2022-07-05 14:52:45 -0700, Andres Freund wrote: > On 2022-07-04 11:01:01 +0900, Masahiko Sawada wrote: > > I've attached the patch, fix_drop_subscriptions_stats.patch, to fix it. > > LGTM. Unless somebody sees a reason not to, I'm planning to commit that to 15 > and HEAD. Pushed.
Re: Issue with pg_stat_subscription_stats
On Thu, Jul 7, 2022 at 12:53 AM Andres Freund wrote: > > Hi, > > On 2022-07-06 11:41:46 +0900, Masahiko Sawada wrote: > > diff --git a/src/test/regress/sql/subscription.sql > > b/src/test/regress/sql/subscription.sql > > index 74c38ead5d..6a46956f6e 100644 > > --- a/src/test/regress/sql/subscription.sql > > +++ b/src/test/regress/sql/subscription.sql > > @@ -30,6 +30,12 @@ CREATE SUBSCRIPTION regress_testsub CONNECTION > > 'dbname=regress_doesnotexist' PUB > > COMMENT ON SUBSCRIPTION regress_testsub IS 'test subscription'; > > SELECT obj_description(s.oid, 'pg_subscription') FROM pg_subscription s; > > > > +-- Check if the subscription stats are created and stats_reset is updated > > +-- by pg_stat_reset_subscription_stats(). > > +SELECT subname, stats_reset IS NULL stats_reset_is_null FROM > > pg_stat_subscription_stats ORDER BY 1; > > Why use ORDER BY 1 instead of just getting the stats for the subscription we > want to test? Seems a bit more robust to show only that one, so we don't get > unnecessary changes if the test needs to create another subscription or such. Right, it's more robust. I've updated the patch accordingly. > > > > +SELECT pg_stat_reset_subscription_stats(oid) FROM pg_subscription; > > +SELECT subname, stats_reset IS NULL stats_reset_is_null FROM > > pg_stat_subscription_stats ORDER BY 1; > > + > > Perhaps worth resetting again and checking that the timestamp is bigger than > the previous timestamp? You can do that with \gset etc. Agreed. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ v3-0001-Create-subscription-stats-entry-at-CREATE-SUBSCRI.patch Description: Binary data
Re: Issue with pg_stat_subscription_stats
On Thu, Jul 7, 2022 at 1:28 AM Andres Freund wrote: > > On 2022-07-05 14:52:45 -0700, Andres Freund wrote: > > On 2022-07-04 11:01:01 +0900, Masahiko Sawada wrote: > > > I've attached the patch, fix_drop_subscriptions_stats.patch, to fix it. > > > > LGTM. Unless somebody sees a reason not to, I'm planning to commit that to > > 15 > > and HEAD. > > Pushed. Thanks! Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Issue with pg_stat_subscription_stats
On 2022-07-07 10:50:27 +0900, Masahiko Sawada wrote: > Right, it's more robust. I've updated the patch accordingly. Do others have thoughts about backpatching this to 15 or not?
Re: Issue with pg_stat_subscription_stats
On Tue, Jul 12, 2022 at 3:26 AM Andres Freund wrote: > > On 2022-07-07 10:50:27 +0900, Masahiko Sawada wrote: > > Right, it's more robust. I've updated the patch accordingly. > > Do others have thoughts about backpatching this to 15 or not? > I am not against backpatching this but OTOH it doesn't appear critical enough to block one's work, so not backpatching should be fine. -- With Regards, Amit Kapila.
Re: Issue with pg_stat_subscription_stats
Hi, On Wed, Mar 16, 2022 at 11:34 PM Masahiko Sawada wrote: > > On Wed, Mar 16, 2022 at 8:51 PM Amit Kapila wrote: > > > > On Tue, Mar 15, 2022 at 10:09 AM Masahiko Sawada > > wrote: > > > > > > On Tue, Mar 15, 2022 at 3:34 AM Melanie Plageman > > > wrote: > > > > > > > > On Mon, Mar 14, 2022 at 4:02 AM Masahiko Sawada > > > > wrote: > > > > > > > > > > On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman > > > > > wrote: > > > > > > > > > > > > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund > > > > > > wrote: > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > > > > > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > So, I noticed that pg_stat_reset_subscription_stats() wasn't > > > > > > > > > working > > > > > > > > > properly, and, upon further investigation, I'm not sure the > > > > > > > > > view > > > > > > > > > pg_stat_subscription_stats is being properly populated. > > > > > > > > > > > > > > > > > > > > > > > > > I have tried the below scenario based on this: > > > > > > > > Step:1 Create some data that generates conflicts and lead to > > > > > > > > apply > > > > > > > > failures and then check in the view: > > > > > > > > > > > > > > I think the problem is present when there was *no* conflict > > > > > > > previously. Because nothing populates the stats entry without an > > > > > > > error, the > > > > > > > reset doesn't have anything to set the stats_reset field in, > > > > > > > which then means > > > > > > > that the stats_reset field is NULL even though stats have been > > > > > > > reset. > > > > > > > > > > > > Yes, this is what I meant. stats_reset is not initialized and > > > > > > without > > > > > > any conflict happening to populate the stats, after resetting the > > > > > > stats, > > > > > > the field still does not get populated. I think this is a bit > > > > > > unexpected. > > > > > > > > > > > > psql (15devel) > > > > > > Type "help" for help. > > > > > > > > > > > > mplageman=# select * from pg_stat_subscription_stats ; > > > > > > subid | subname | apply_error_count | sync_error_count | > > > > > > stats_reset > > > > > > ---+-+---+--+- > > > > > > 16398 | mysub | 0 |0 | > > > > > > (1 row) > > > > > > > > > > > > mplageman=# select pg_stat_reset_subscription_stats(16398); > > > > > > pg_stat_reset_subscription_stats > > > > > > -- > > > > > > > > > > > > (1 row) > > > > > > > > > > > > mplageman=# select * from pg_stat_subscription_stats ; > > > > > > subid | subname | apply_error_count | sync_error_count | > > > > > > stats_reset > > > > > > ---+-+---+--+- > > > > > > 16398 | mysub | 0 |0 | > > > > > > (1 row) > > > > > > > > > > > > > > > > Looking at other statistics such as replication slots, shared stats, > > > > > and SLRU stats, it makes sense that resetting it populates the stats. > > > > > So we need to fix this issue. > > > > > > > > > > However, I think the proposed fix has two problems; it can create an > > > > > entry for non-existing subscriptions if the user directly calls > > > > > function pg_stat_get_subscription_stats(), and stats_reset value is > > > > > not updated in the stats file as it is not done by the stats > > > > > collector. > > > > > > > > You are right. My initial patch was incorrect. > > > > > > > > Thinking about it more, the initial behavior is technically the same for > > > > pg_stat_database. It is just that I didn't notice because you end up > > > > creating stats for pg_stat_database so quickly that you usually never > > > > see them before. > > > > > > > > In pg_stat_get_db_stat_reset_time(): > > > > > > > > if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL) > > > > result = 0; > > > > else > > > > result = dbentry->stat_reset_timestamp; > > > > > > > > if (result == 0) > > > > PG_RETURN_NULL(); > > > > else > > > > PG_RETURN_TIMESTAMPTZ(result); > > > > > > > > and in pgstat_recv_resetcounter(): > > > > > > > > dbentry = pgstat_get_db_entry(msg->m_databaseid, false); > > > > > > > > if (!dbentry) > > > > return; > > > > > > > > Thinking about it now, though, maybe an alternative solution would be to > > > > have all columns or all columns except the subid/subname or dbname/dboid > > > > be NULL until the statistics have been created, at which point the > > > > reset_timestamp is populated with the current timestamp. > > > > > > It's true that stats_reset is NULL if the statistics of database are > > > not created yet. > > > > > > > So, if the behavior is the same as pg_stat_database, do we really want > > to change anything in this regard? > > Both pg_stat_database and pg_stat_subscription_stats work similarly i
Re: Issue with pg_stat_subscription_stats
On Fri, Jul 1, 2022 at 7:12 AM Masahiko Sawada wrote: > > On Wed, Mar 16, 2022 at 11:34 PM Masahiko Sawada > wrote: > > > > While looking at this issue again, I realized there seems to be two > problems with subscription stats on shmem stats: > > Firstly, we call pgstat_create_subscription() when creating a > subscription but the subscription stats are reported by apply workers. > And pgstat_create_subscription() just calls > pgstat_create_transactional(): > > void > pgstat_create_subscription(Oid subid) > { > pgstat_create_transactional(PGSTAT_KIND_SUBSCRIPTION, > InvalidOid, subid); > } > > I guess calling pgstat_create_subscription() is not necessary for the > current usage. On the other hand, if we create the subscription stats > there we can resolve the issue Melanie reported in this thread. > It won't create the stats entry in the shared hash table, so the behavior should be the same as without shared stats. I am not sure we need to do anything for this one. > The second problem is that the following code in DropSubscription() > should be updated: > > /* > * Tell the cumulative stats system that the subscription is getting > * dropped. We can safely report dropping the subscription statistics here > * if the subscription is associated with a replication slot since we > * cannot run DROP SUBSCRIPTION inside a transaction block. Subscription > * statistics will be removed later by (auto)vacuum either if it's not > * associated with a replication slot or if the message for dropping the > * subscription gets lost. > */ > if (slotname) > pgstat_drop_subscription(subid); > > I think we can call pgstat_drop_subscription() even if slotname is > NULL and need to update the comment. > +1. > IIUC autovacuum is no longer > responsible for garbage collection. > Right, this is my understanding as well. -- With Regards, Amit Kapila.
Re: Issue with pg_stat_subscription_stats
On Fri, Jul 1, 2022 at 3:01 PM Amit Kapila wrote: > > On Fri, Jul 1, 2022 at 7:12 AM Masahiko Sawada wrote: > > > > On Wed, Mar 16, 2022 at 11:34 PM Masahiko Sawada > > wrote: > > > > > > > While looking at this issue again, I realized there seems to be two > > problems with subscription stats on shmem stats: > > > > Firstly, we call pgstat_create_subscription() when creating a > > subscription but the subscription stats are reported by apply workers. > > And pgstat_create_subscription() just calls > > pgstat_create_transactional(): > > > > void > > pgstat_create_subscription(Oid subid) > > { > > pgstat_create_transactional(PGSTAT_KIND_SUBSCRIPTION, > > InvalidOid, subid); > > } > > > > I guess calling pgstat_create_subscription() is not necessary for the > > current usage. On the other hand, if we create the subscription stats > > there we can resolve the issue Melanie reported in this thread. > > > > It won't create the stats entry in the shared hash table, so the > behavior should be the same as without shared stats. Yes, my point is that it may be misleading that the subscription stats are created when a subscription is created. The initial behavior is technically the same for pg_stat_database. That is, we don't create the stats entry for them when creating the object. But we don’t call pgstat_create_transactional when creating a database (we don’t have a function like pgstat_create_database()) whereas we do for subscription stats. On the other hand, I'm not sure we agreed that the behavior that Melanie reported is not a problem. The user might get confused since the subscription stats works differently than other stats when a reset. Previously, the primary reason why I hesitated to create the subscription stats when creating a subscription is that CREATE SUBSCRIPTION (with create_slot = false) can be rolled back. But with the shmem stats, we can easily resolve it by using pgstat_create_transactional(). > > > The second problem is that the following code in DropSubscription() > > should be updated: > > > > /* > > * Tell the cumulative stats system that the subscription is getting > > * dropped. We can safely report dropping the subscription statistics > > here > > * if the subscription is associated with a replication slot since we > > * cannot run DROP SUBSCRIPTION inside a transaction block. > > Subscription > > * statistics will be removed later by (auto)vacuum either if it's not > > * associated with a replication slot or if the message for dropping the > > * subscription gets lost. > > */ > > if (slotname) > > pgstat_drop_subscription(subid); > > > > I think we can call pgstat_drop_subscription() even if slotname is > > NULL and need to update the comment. > > > > +1. > > > IIUC autovacuum is no longer > > responsible for garbage collection. > > > > Right, this is my understanding as well. Thank you for the confirmation. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Issue with pg_stat_subscription_stats
Hi, On 2022-07-01 10:41:55 +0900, Masahiko Sawada wrote: > While looking at this issue again, I realized there seems to be two > problems with subscription stats on shmem stats: > > Firstly, we call pgstat_create_subscription() when creating a > subscription but the subscription stats are reported by apply workers. Why is it relevant where the stats are reported? > And pgstat_create_subscription() just calls > pgstat_create_transactional(): > > void > pgstat_create_subscription(Oid subid) > { > pgstat_create_transactional(PGSTAT_KIND_SUBSCRIPTION, > InvalidOid, subid); > } > > I guess calling pgstat_create_subscription() is not necessary for the > current usage. It ensures that the stats are dropped if the subscription fails to be created partway through / the transaction is aborted. There's probably no way for that to happen today, but it still seems the right thing. > On the other hand, if we create the subscription stats > there we can resolve the issue Melanie reported in this thread. I am confused what the place of creation addresses? > The second problem is that the following code in DropSubscription() > should be updated: > > /* > * Tell the cumulative stats system that the subscription is getting > * dropped. We can safely report dropping the subscription statistics here > * if the subscription is associated with a replication slot since we > * cannot run DROP SUBSCRIPTION inside a transaction block. Subscription > * statistics will be removed later by (auto)vacuum either if it's not > * associated with a replication slot or if the message for dropping the > * subscription gets lost. > */ > if (slotname) > pgstat_drop_subscription(subid); > > I think we can call pgstat_drop_subscription() even if slotname is > NULL and need to update the comment. IIUC autovacuum is no longer > responsible for garbage collection. Yep, that needs to be updated. Greetings, Andres Freund
Re: Issue with pg_stat_subscription_stats
Hi, On 2022-07-01 16:08:48 +0900, Masahiko Sawada wrote: > Yes, my point is that it may be misleading that the subscription stats > are created when a subscription is created. I think it's important to create stats at that time, because otherwise it's basically impossible to ensure that stats are dropped when a transaction rolls back. If some / all columns should return something else before stats are reported that can be addressed easily by tracking that in a separate field. > On the other hand, I'm not sure we agreed that the behavior that > Melanie reported is not a problem. The user might get confused since > the subscription stats works differently than other stats when a > reset. Previously, the primary reason why I hesitated to create the > subscription stats when creating a subscription is that CREATE > SUBSCRIPTION (with create_slot = false) can be rolled back. But with > the shmem stats, we can easily resolve it by using > pgstat_create_transactional(). Yep. > > > The second problem is that the following code in DropSubscription() > > > should be updated: > > > > > > /* > > > * Tell the cumulative stats system that the subscription is getting > > > * dropped. We can safely report dropping the subscription statistics > > > here > > > * if the subscription is associated with a replication slot since we > > > * cannot run DROP SUBSCRIPTION inside a transaction block. > > > Subscription > > > * statistics will be removed later by (auto)vacuum either if it's not > > > * associated with a replication slot or if the message for dropping > > > the > > > * subscription gets lost. > > > */ > > > if (slotname) > > > pgstat_drop_subscription(subid); > > > > > > I think we can call pgstat_drop_subscription() even if slotname is > > > NULL and need to update the comment. > > > > > > > +1. > > > > > IIUC autovacuum is no longer > > > responsible for garbage collection. > > > > > > > Right, this is my understanding as well. > > Thank you for the confirmation. Want to propose a patch? Greetings, Andres Freund
Re: Issue with pg_stat_subscription_stats
On Sat, Jul 2, 2022 at 2:53 Andres Freund wrote: > Hi, > > On 2022-07-01 16:08:48 +0900, Masahiko Sawada wrote: > > Yes, my point is that it may be misleading that the subscription stats > > are created when a subscription is created. > > I think it's important to create stats at that time, because otherwise it's > basically impossible to ensure that stats are dropped when a transaction > rolls > back. If some / all columns should return something else before stats are > reported that can be addressed easily by tracking that in a separate field. > > > > On the other hand, I'm not sure we agreed that the behavior that > > Melanie reported is not a problem. The user might get confused since > > the subscription stats works differently than other stats when a > > reset. Previously, the primary reason why I hesitated to create the > > subscription stats when creating a subscription is that CREATE > > SUBSCRIPTION (with create_slot = false) can be rolled back. But with > > the shmem stats, we can easily resolve it by using > > pgstat_create_transactional(). > > Yep. > > > > > > The second problem is that the following code in DropSubscription() > > > > should be updated: > > > > > > > > /* > > > > * Tell the cumulative stats system that the subscription is > getting > > > > * dropped. We can safely report dropping the subscription > statistics here > > > > * if the subscription is associated with a replication slot > since we > > > > * cannot run DROP SUBSCRIPTION inside a transaction block. > Subscription > > > > * statistics will be removed later by (auto)vacuum either if > it's not > > > > * associated with a replication slot or if the message for > dropping the > > > > * subscription gets lost. > > > > */ > > > > if (slotname) > > > > pgstat_drop_subscription(subid); > > > > > > > > I think we can call pgstat_drop_subscription() even if slotname is > > > > NULL and need to update the comment. > > > > > > > > > > +1. > > > > > > > IIUC autovacuum is no longer > > > > responsible for garbage collection. > > > > > > > > > > Right, this is my understanding as well. > > > > Thank you for the confirmation. > > Want to propose a patch? Yes, I’ll propose a patch. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Issue with pg_stat_subscription_stats
On Sat, Jul 2, 2022 at 9:52 AM Masahiko Sawada wrote: > > > > On Sat, Jul 2, 2022 at 2:53 Andres Freund wrote: >> >> Hi, >> >> On 2022-07-01 16:08:48 +0900, Masahiko Sawada wrote: >> > Yes, my point is that it may be misleading that the subscription stats >> > are created when a subscription is created. >> >> I think it's important to create stats at that time, because otherwise it's >> basically impossible to ensure that stats are dropped when a transaction >> rolls >> back. If some / all columns should return something else before stats are >> reported that can be addressed easily by tracking that in a separate field. >> >> >> > On the other hand, I'm not sure we agreed that the behavior that >> > Melanie reported is not a problem. The user might get confused since >> > the subscription stats works differently than other stats when a >> > reset. Previously, the primary reason why I hesitated to create the >> > subscription stats when creating a subscription is that CREATE >> > SUBSCRIPTION (with create_slot = false) can be rolled back. But with >> > the shmem stats, we can easily resolve it by using >> > pgstat_create_transactional(). >> >> Yep. >> >> >> > > > The second problem is that the following code in DropSubscription() >> > > > should be updated: >> > > > >> > > > /* >> > > > * Tell the cumulative stats system that the subscription is >> > > > getting >> > > > * dropped. We can safely report dropping the subscription >> > > > statistics here >> > > > * if the subscription is associated with a replication slot since >> > > > we >> > > > * cannot run DROP SUBSCRIPTION inside a transaction block. >> > > > Subscription >> > > > * statistics will be removed later by (auto)vacuum either if it's >> > > > not >> > > > * associated with a replication slot or if the message for >> > > > dropping the >> > > > * subscription gets lost. >> > > > */ >> > > > if (slotname) >> > > > pgstat_drop_subscription(subid); >> > > > >> > > > I think we can call pgstat_drop_subscription() even if slotname is >> > > > NULL and need to update the comment. >> > > > >> > > >> > > +1. >> > > >> > > > IIUC autovacuum is no longer >> > > > responsible for garbage collection. >> > > > >> > > >> > > Right, this is my understanding as well. >> > >> > Thank you for the confirmation. >> >> Want to propose a patch? > > > Yes, I’ll propose a patch. > I've attached the patch, fix_drop_subscriptions_stats.patch, to fix it. I've also attached another PoC patch, poc_create_subscription_stats.patch, to create the stats entry when creating the subscription, which address the issue reported in this thread; the pg_stat_reset_subscription_stats() doesn't update the stats_reset if no error is reported yet. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ fix_drop_subscription_stats.patch Description: Binary data poc_create_subscription_stats.patch Description: Binary data
Re: Issue with pg_stat_subscription_stats
On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman wrote: > > So, I noticed that pg_stat_reset_subscription_stats() wasn't working > properly, and, upon further investigation, I'm not sure the view > pg_stat_subscription_stats is being properly populated. > I have tried the below scenario based on this: Step:1 Create some data that generates conflicts and lead to apply failures and then check in the view: postgres=# select * from pg_stat_subscription_stats; subid | subname | apply_error_count | sync_error_count | stats_reset ---+-+---+--+- 16389 | sub1| 4 |0 | (1 row) Step-2: Reset the view postgres=# select * from pg_stat_reset_subscription_stats(16389); pg_stat_reset_subscription_stats -- (1 row) Step-3: Again, check the view: postgres=# select * from pg_stat_subscription_stats; subid | subname | apply_error_count | sync_error_count | stats_reset ---+-+---+--+-- 16389 | sub1| 0 |0 | 2022-03-12 08:21:39.156971+05:30 (1 row) The stats_reset time seems to be populated. Similarly, I have tried by passing NULL to pg_stat_reset_subscription_stats and it works. I think I am missing something here, can you please explain the exact scenario/steps where you observed that this API is not working. -- With Regards, Amit Kapila.
Re: Issue with pg_stat_subscription_stats
Hi, On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > wrote: > > > > So, I noticed that pg_stat_reset_subscription_stats() wasn't working > > properly, and, upon further investigation, I'm not sure the view > > pg_stat_subscription_stats is being properly populated. > > > > I have tried the below scenario based on this: > Step:1 Create some data that generates conflicts and lead to apply > failures and then check in the view: I think the problem is present when there was *no* conflict previously. Because nothing populates the stats entry without an error, the reset doesn't have anything to set the stats_reset field in, which then means that the stats_reset field is NULL even though stats have been reset. I'll just repeat what I've said before: Making variable numbered stats individiually resettable is a bad idea. Greetings, Andres Freund
Re: Issue with pg_stat_subscription_stats
On Sat, Mar 12, 2022 at 3:15 PM Andres Freund wrote: > > Hi, > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > wrote: > > > > > > So, I noticed that pg_stat_reset_subscription_stats() wasn't working > > > properly, and, upon further investigation, I'm not sure the view > > > pg_stat_subscription_stats is being properly populated. > > > > > > > I have tried the below scenario based on this: > > Step:1 Create some data that generates conflicts and lead to apply > > failures and then check in the view: > > I think the problem is present when there was *no* conflict > previously. Because nothing populates the stats entry without an error, the > reset doesn't have anything to set the stats_reset field in, which then means > that the stats_reset field is NULL even though stats have been reset. Yes, this is what I meant. stats_reset is not initialized and without any conflict happening to populate the stats, after resetting the stats, the field still does not get populated. I think this is a bit unexpected. psql (15devel) Type "help" for help. mplageman=# select * from pg_stat_subscription_stats ; subid | subname | apply_error_count | sync_error_count | stats_reset ---+-+---+--+- 16398 | mysub | 0 |0 | (1 row) mplageman=# select pg_stat_reset_subscription_stats(16398); pg_stat_reset_subscription_stats -- (1 row) mplageman=# select * from pg_stat_subscription_stats ; subid | subname | apply_error_count | sync_error_count | stats_reset ---+-+---+--+- 16398 | mysub | 0 |0 | (1 row) - Melanie
Re: Issue with pg_stat_subscription_stats
On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman wrote: > > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund wrote: > > > > Hi, > > > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > > wrote: > > > > > > > > So, I noticed that pg_stat_reset_subscription_stats() wasn't working > > > > properly, and, upon further investigation, I'm not sure the view > > > > pg_stat_subscription_stats is being properly populated. > > > > > > > > > > I have tried the below scenario based on this: > > > Step:1 Create some data that generates conflicts and lead to apply > > > failures and then check in the view: > > > > I think the problem is present when there was *no* conflict > > previously. Because nothing populates the stats entry without an error, the > > reset doesn't have anything to set the stats_reset field in, which then > > means > > that the stats_reset field is NULL even though stats have been reset. > > Yes, this is what I meant. stats_reset is not initialized and without > any conflict happening to populate the stats, after resetting the stats, > the field still does not get populated. I think this is a bit > unexpected. > > psql (15devel) > Type "help" for help. > > mplageman=# select * from pg_stat_subscription_stats ; > subid | subname | apply_error_count | sync_error_count | stats_reset > ---+-+---+--+- > 16398 | mysub | 0 |0 | > (1 row) > > mplageman=# select pg_stat_reset_subscription_stats(16398); > pg_stat_reset_subscription_stats > -- > > (1 row) > > mplageman=# select * from pg_stat_subscription_stats ; > subid | subname | apply_error_count | sync_error_count | stats_reset > ---+-+---+--+- > 16398 | mysub | 0 |0 | > (1 row) > Looking at other statistics such as replication slots, shared stats, and SLRU stats, it makes sense that resetting it populates the stats. So we need to fix this issue. However, I think the proposed fix has two problems; it can create an entry for non-existing subscriptions if the user directly calls function pg_stat_get_subscription_stats(), and stats_reset value is not updated in the stats file as it is not done by the stats collector. An alternative solution would be to send the message for creating the subscription at the end of CRAETE SUBSCRIPTION which basically resolves them. A caveat is that if CREATE SUBSCRIPTION (that doesn't involve replication slot creation) is rolled back, the first problem still occurs. But it should not practically matter as a similar thing is possible via existing table-related functions for dropped tables. Also, we normally don't know the OID of subscription that is rolled back. I've attached a patch for that. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 3922658bbc..e3a878e7cc 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -611,6 +611,14 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt, InvokeObjectPostCreateHook(SubscriptionRelationId, subid, 0); + /* + * Send a message for creating the subscription to the stats collector. + * CREATE SUBSCRIPTION that doesn't involve replication slot creation + * might be rolled back but these stats won't be shown on the + * pg_stat_subscription_stats view and removed later by (auto)vacuum. + */ + pgstat_report_subscription_create(subid); + return myself; } diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index 53ddd930e6..7a6bad0032 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -380,7 +380,7 @@ static void pgstat_recv_connect(PgStat_MsgConnect *msg, int len); static void pgstat_recv_disconnect(PgStat_MsgDisconnect *msg, int len); static void pgstat_recv_replslot(PgStat_MsgReplSlot *msg, int len); static void pgstat_recv_tempfile(PgStat_MsgTempFile *msg, int len); -static void pgstat_recv_subscription_drop(PgStat_MsgSubscriptionDrop *msg, int len); +static void pgstat_recv_subscription(PgStat_MsgSubscription *msg, int len); static void pgstat_recv_subscription_error(PgStat_MsgSubscriptionError *msg, int len); /* @@ -1945,6 +1945,23 @@ pgstat_report_subscription_error(Oid subid, bool is_apply_error) pgstat_send(&msg, sizeof(PgStat_MsgSubscriptionError)); } +/* -- + * pgstat_report_subscription_create() - + * + * Tell the collector about creating the subscription. + * -- + */ +void +pgstat_report_subscription_create(Oid subid) +{ + PgStat_MsgSubscription msg; + + pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_SUBSCRIPTION); + msg.m_subid = subid; + msg.m_create = true; + pgstat
Re: Issue with pg_stat_subscription_stats
On Mon, Mar 14, 2022 at 4:02 AM Masahiko Sawada wrote: > > On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman > wrote: > > > > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund wrote: > > > > > > Hi, > > > > > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > > > wrote: > > > > > > > > > > So, I noticed that pg_stat_reset_subscription_stats() wasn't working > > > > > properly, and, upon further investigation, I'm not sure the view > > > > > pg_stat_subscription_stats is being properly populated. > > > > > > > > > > > > > I have tried the below scenario based on this: > > > > Step:1 Create some data that generates conflicts and lead to apply > > > > failures and then check in the view: > > > > > > I think the problem is present when there was *no* conflict > > > previously. Because nothing populates the stats entry without an error, > > > the > > > reset doesn't have anything to set the stats_reset field in, which then > > > means > > > that the stats_reset field is NULL even though stats have been reset. > > > > Yes, this is what I meant. stats_reset is not initialized and without > > any conflict happening to populate the stats, after resetting the stats, > > the field still does not get populated. I think this is a bit > > unexpected. > > > > psql (15devel) > > Type "help" for help. > > > > mplageman=# select * from pg_stat_subscription_stats ; > > subid | subname | apply_error_count | sync_error_count | stats_reset > > ---+-+---+--+- > > 16398 | mysub | 0 |0 | > > (1 row) > > > > mplageman=# select pg_stat_reset_subscription_stats(16398); > > pg_stat_reset_subscription_stats > > -- > > > > (1 row) > > > > mplageman=# select * from pg_stat_subscription_stats ; > > subid | subname | apply_error_count | sync_error_count | stats_reset > > ---+-+---+--+- > > 16398 | mysub | 0 |0 | > > (1 row) > > > > Looking at other statistics such as replication slots, shared stats, > and SLRU stats, it makes sense that resetting it populates the stats. > So we need to fix this issue. > > However, I think the proposed fix has two problems; it can create an > entry for non-existing subscriptions if the user directly calls > function pg_stat_get_subscription_stats(), and stats_reset value is > not updated in the stats file as it is not done by the stats > collector. You are right. My initial patch was incorrect. Thinking about it more, the initial behavior is technically the same for pg_stat_database. It is just that I didn't notice because you end up creating stats for pg_stat_database so quickly that you usually never see them before. In pg_stat_get_db_stat_reset_time(): if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL) result = 0; else result = dbentry->stat_reset_timestamp; if (result == 0) PG_RETURN_NULL(); else PG_RETURN_TIMESTAMPTZ(result); and in pgstat_recv_resetcounter(): dbentry = pgstat_get_db_entry(msg->m_databaseid, false); if (!dbentry) return; Thinking about it now, though, maybe an alternative solution would be to have all columns or all columns except the subid/subname or dbname/dboid be NULL until the statistics have been created, at which point the reset_timestamp is populated with the current timestamp. mplageman=# select * from pg_stat_subscription_stats ; subid | subname | apply_error_count | sync_error_count | stats_reset ---+-+---+--+- 16397 | foosub | | | 16408 | barsub | | | (2 rows) All resetting before the stats are created would be a no-op. - Melanie
Re: Issue with pg_stat_subscription_stats
On Sun, Mar 13, 2022 at 1:45 AM Andres Freund wrote: > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > wrote: > > > > > > So, I noticed that pg_stat_reset_subscription_stats() wasn't working > > > properly, and, upon further investigation, I'm not sure the view > > > pg_stat_subscription_stats is being properly populated. > > > > > > > I have tried the below scenario based on this: > > Step:1 Create some data that generates conflicts and lead to apply > > failures and then check in the view: > > I think the problem is present when there was *no* conflict > previously. Because nothing populates the stats entry without an error, the > reset doesn't have anything to set the stats_reset field in, which then means > that the stats_reset field is NULL even though stats have been reset. > > I'll just repeat what I've said before: Making variable numbered stats > individiually resettable is a bad idea. > IIUC correctly, we are doing this via pg_stat_reset_single_table_counters(), pg_stat_reset_single_function_counters(), pg_stat_reset_replication_slot(), pg_stat_reset_subscription_stats(). So, if we want to do something in this regrard then it is probably better to do for all. -- With Regards, Amit Kapila.
Re: Issue with pg_stat_subscription_stats
On Tue, Mar 15, 2022 at 3:34 AM Melanie Plageman wrote: > > On Mon, Mar 14, 2022 at 4:02 AM Masahiko Sawada wrote: > > > > On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman > > wrote: > > > > > > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund wrote: > > > > > > > > Hi, > > > > > > > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > > > > wrote: > > > > > > > > > > > > So, I noticed that pg_stat_reset_subscription_stats() wasn't working > > > > > > properly, and, upon further investigation, I'm not sure the view > > > > > > pg_stat_subscription_stats is being properly populated. > > > > > > > > > > > > > > > > I have tried the below scenario based on this: > > > > > Step:1 Create some data that generates conflicts and lead to apply > > > > > failures and then check in the view: > > > > > > > > I think the problem is present when there was *no* conflict > > > > previously. Because nothing populates the stats entry without an error, > > > > the > > > > reset doesn't have anything to set the stats_reset field in, which then > > > > means > > > > that the stats_reset field is NULL even though stats have been reset. > > > > > > Yes, this is what I meant. stats_reset is not initialized and without > > > any conflict happening to populate the stats, after resetting the stats, > > > the field still does not get populated. I think this is a bit > > > unexpected. > > > > > > psql (15devel) > > > Type "help" for help. > > > > > > mplageman=# select * from pg_stat_subscription_stats ; > > > subid | subname | apply_error_count | sync_error_count | stats_reset > > > ---+-+---+--+- > > > 16398 | mysub | 0 |0 | > > > (1 row) > > > > > > mplageman=# select pg_stat_reset_subscription_stats(16398); > > > pg_stat_reset_subscription_stats > > > -- > > > > > > (1 row) > > > > > > mplageman=# select * from pg_stat_subscription_stats ; > > > subid | subname | apply_error_count | sync_error_count | stats_reset > > > ---+-+---+--+- > > > 16398 | mysub | 0 |0 | > > > (1 row) > > > > > > > Looking at other statistics such as replication slots, shared stats, > > and SLRU stats, it makes sense that resetting it populates the stats. > > So we need to fix this issue. > > > > However, I think the proposed fix has two problems; it can create an > > entry for non-existing subscriptions if the user directly calls > > function pg_stat_get_subscription_stats(), and stats_reset value is > > not updated in the stats file as it is not done by the stats > > collector. > > You are right. My initial patch was incorrect. > > Thinking about it more, the initial behavior is technically the same for > pg_stat_database. It is just that I didn't notice because you end up > creating stats for pg_stat_database so quickly that you usually never > see them before. > > In pg_stat_get_db_stat_reset_time(): > > if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL) > result = 0; > else > result = dbentry->stat_reset_timestamp; > > if (result == 0) > PG_RETURN_NULL(); > else > PG_RETURN_TIMESTAMPTZ(result); > > and in pgstat_recv_resetcounter(): > > dbentry = pgstat_get_db_entry(msg->m_databaseid, false); > > if (!dbentry) > return; > > Thinking about it now, though, maybe an alternative solution would be to > have all columns or all columns except the subid/subname or dbname/dboid > be NULL until the statistics have been created, at which point the > reset_timestamp is populated with the current timestamp. It's true that stats_reset is NULL if the statistics of database are not created yet. But looking at other columns such as tup_deleted, they show 0 in the case. So having all columns or all counter columns in pg_stat_subscription_stats be NULL would not be consistent with other statistics, which I think is not a good idea. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Issue with pg_stat_subscription_stats
On Tue, Mar 15, 2022 at 10:09 AM Masahiko Sawada wrote: > > On Tue, Mar 15, 2022 at 3:34 AM Melanie Plageman > wrote: > > > > On Mon, Mar 14, 2022 at 4:02 AM Masahiko Sawada > > wrote: > > > > > > On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman > > > wrote: > > > > > > > > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund > > > > wrote: > > > > > > > > > > Hi, > > > > > > > > > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > > > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > > > > > wrote: > > > > > > > > > > > > > > So, I noticed that pg_stat_reset_subscription_stats() wasn't > > > > > > > working > > > > > > > properly, and, upon further investigation, I'm not sure the view > > > > > > > pg_stat_subscription_stats is being properly populated. > > > > > > > > > > > > > > > > > > > I have tried the below scenario based on this: > > > > > > Step:1 Create some data that generates conflicts and lead to apply > > > > > > failures and then check in the view: > > > > > > > > > > I think the problem is present when there was *no* conflict > > > > > previously. Because nothing populates the stats entry without an > > > > > error, the > > > > > reset doesn't have anything to set the stats_reset field in, which > > > > > then means > > > > > that the stats_reset field is NULL even though stats have been reset. > > > > > > > > Yes, this is what I meant. stats_reset is not initialized and without > > > > any conflict happening to populate the stats, after resetting the stats, > > > > the field still does not get populated. I think this is a bit > > > > unexpected. > > > > > > > > psql (15devel) > > > > Type "help" for help. > > > > > > > > mplageman=# select * from pg_stat_subscription_stats ; > > > > subid | subname | apply_error_count | sync_error_count | stats_reset > > > > ---+-+---+--+- > > > > 16398 | mysub | 0 |0 | > > > > (1 row) > > > > > > > > mplageman=# select pg_stat_reset_subscription_stats(16398); > > > > pg_stat_reset_subscription_stats > > > > -- > > > > > > > > (1 row) > > > > > > > > mplageman=# select * from pg_stat_subscription_stats ; > > > > subid | subname | apply_error_count | sync_error_count | stats_reset > > > > ---+-+---+--+- > > > > 16398 | mysub | 0 |0 | > > > > (1 row) > > > > > > > > > > Looking at other statistics such as replication slots, shared stats, > > > and SLRU stats, it makes sense that resetting it populates the stats. > > > So we need to fix this issue. > > > > > > However, I think the proposed fix has two problems; it can create an > > > entry for non-existing subscriptions if the user directly calls > > > function pg_stat_get_subscription_stats(), and stats_reset value is > > > not updated in the stats file as it is not done by the stats > > > collector. > > > > You are right. My initial patch was incorrect. > > > > Thinking about it more, the initial behavior is technically the same for > > pg_stat_database. It is just that I didn't notice because you end up > > creating stats for pg_stat_database so quickly that you usually never > > see them before. > > > > In pg_stat_get_db_stat_reset_time(): > > > > if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL) > > result = 0; > > else > > result = dbentry->stat_reset_timestamp; > > > > if (result == 0) > > PG_RETURN_NULL(); > > else > > PG_RETURN_TIMESTAMPTZ(result); > > > > and in pgstat_recv_resetcounter(): > > > > dbentry = pgstat_get_db_entry(msg->m_databaseid, false); > > > > if (!dbentry) > > return; > > > > Thinking about it now, though, maybe an alternative solution would be to > > have all columns or all columns except the subid/subname or dbname/dboid > > be NULL until the statistics have been created, at which point the > > reset_timestamp is populated with the current timestamp. > > It's true that stats_reset is NULL if the statistics of database are > not created yet. > So, if the behavior is the same as pg_stat_database, do we really want to change anything in this regard? -- With Regards, Amit Kapila.
Re: Issue with pg_stat_subscription_stats
On Wed, Mar 16, 2022 at 8:51 PM Amit Kapila wrote: > > On Tue, Mar 15, 2022 at 10:09 AM Masahiko Sawada > wrote: > > > > On Tue, Mar 15, 2022 at 3:34 AM Melanie Plageman > > wrote: > > > > > > On Mon, Mar 14, 2022 at 4:02 AM Masahiko Sawada > > > wrote: > > > > > > > > On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman > > > > wrote: > > > > > > > > > > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund > > > > > wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > > > > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > > > > > > wrote: > > > > > > > > > > > > > > > > So, I noticed that pg_stat_reset_subscription_stats() wasn't > > > > > > > > working > > > > > > > > properly, and, upon further investigation, I'm not sure the view > > > > > > > > pg_stat_subscription_stats is being properly populated. > > > > > > > > > > > > > > > > > > > > > > I have tried the below scenario based on this: > > > > > > > Step:1 Create some data that generates conflicts and lead to apply > > > > > > > failures and then check in the view: > > > > > > > > > > > > I think the problem is present when there was *no* conflict > > > > > > previously. Because nothing populates the stats entry without an > > > > > > error, the > > > > > > reset doesn't have anything to set the stats_reset field in, which > > > > > > then means > > > > > > that the stats_reset field is NULL even though stats have been > > > > > > reset. > > > > > > > > > > Yes, this is what I meant. stats_reset is not initialized and without > > > > > any conflict happening to populate the stats, after resetting the > > > > > stats, > > > > > the field still does not get populated. I think this is a bit > > > > > unexpected. > > > > > > > > > > psql (15devel) > > > > > Type "help" for help. > > > > > > > > > > mplageman=# select * from pg_stat_subscription_stats ; > > > > > subid | subname | apply_error_count | sync_error_count | stats_reset > > > > > ---+-+---+--+- > > > > > 16398 | mysub | 0 |0 | > > > > > (1 row) > > > > > > > > > > mplageman=# select pg_stat_reset_subscription_stats(16398); > > > > > pg_stat_reset_subscription_stats > > > > > -- > > > > > > > > > > (1 row) > > > > > > > > > > mplageman=# select * from pg_stat_subscription_stats ; > > > > > subid | subname | apply_error_count | sync_error_count | stats_reset > > > > > ---+-+---+--+- > > > > > 16398 | mysub | 0 |0 | > > > > > (1 row) > > > > > > > > > > > > > Looking at other statistics such as replication slots, shared stats, > > > > and SLRU stats, it makes sense that resetting it populates the stats. > > > > So we need to fix this issue. > > > > > > > > However, I think the proposed fix has two problems; it can create an > > > > entry for non-existing subscriptions if the user directly calls > > > > function pg_stat_get_subscription_stats(), and stats_reset value is > > > > not updated in the stats file as it is not done by the stats > > > > collector. > > > > > > You are right. My initial patch was incorrect. > > > > > > Thinking about it more, the initial behavior is technically the same for > > > pg_stat_database. It is just that I didn't notice because you end up > > > creating stats for pg_stat_database so quickly that you usually never > > > see them before. > > > > > > In pg_stat_get_db_stat_reset_time(): > > > > > > if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL) > > > result = 0; > > > else > > > result = dbentry->stat_reset_timestamp; > > > > > > if (result == 0) > > > PG_RETURN_NULL(); > > > else > > > PG_RETURN_TIMESTAMPTZ(result); > > > > > > and in pgstat_recv_resetcounter(): > > > > > > dbentry = pgstat_get_db_entry(msg->m_databaseid, false); > > > > > > if (!dbentry) > > > return; > > > > > > Thinking about it now, though, maybe an alternative solution would be to > > > have all columns or all columns except the subid/subname or dbname/dboid > > > be NULL until the statistics have been created, at which point the > > > reset_timestamp is populated with the current timestamp. > > > > It's true that stats_reset is NULL if the statistics of database are > > not created yet. > > > > So, if the behavior is the same as pg_stat_database, do we really want > to change anything in this regard? Both pg_stat_database and pg_stat_subscription_stats work similarly in principle but they work differently in practice since there are more chances to create the database stats entry such as connections, disconnections, and autovacuum than the subscription stats entry. I think that the issue reported by Melanie is valid and perhaps most users would expect the same behavior as other statistics. Regards, -- Masahiko Sawada EDB: https://www