Thread: Issue with pg_stat_subscription_stats
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 don't think subscriptionStatHash will be created properly and that the
reset timestamp won't be initialized without the following code:
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 53ddd930e6..0b8c5436e9 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3092,7 +3092,7 @@ pgstat_fetch_stat_subscription(Oid subid)
/* Load the stats file if needed */
backend_read_statsfile();
- return pgstat_get_subscription_entry(subid, false);
+ return pgstat_get_subscription_entry(subid, true);
}
/*
@@ -6252,7 +6252,7 @@ pgstat_get_subscription_entry(Oid subid, bool create)
/* If not found, initialize the new one */
if (!found)
- pgstat_reset_subscription(subentry, 0);
+ pgstat_reset_subscription(subentry, GetCurrentTimestamp());
return subentry;
}
- melanie
On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman <melanieplageman@gmail.com> 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.
Hi, On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > <melanieplageman@gmail.com> 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
On Sat, Mar 12, 2022 at 3:15 PM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > <melanieplageman@gmail.com> 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
On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman <melanieplageman@gmail.com> wrote: > > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund <andres@anarazel.de> wrote: > > > > Hi, > > > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > > <melanieplageman@gmail.com> 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/
Attachment
On Mon, Mar 14, 2022 at 4:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund <andres@anarazel.de> wrote:
> > >
> > > Hi,
> > >
> > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote:
> > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman
> > > > <melanieplageman@gmail.com> 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
On Sun, Mar 13, 2022 at 1:45 AM Andres Freund <andres@anarazel.de> wrote: > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > <melanieplageman@gmail.com> 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.
On Tue, Mar 15, 2022 at 3:34 AM Melanie Plageman <melanieplageman@gmail.com> wrote: > > On Mon, Mar 14, 2022 at 4:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman > > <melanieplageman@gmail.com> wrote: > > > > > > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund <andres@anarazel.de> wrote: > > > > > > > > Hi, > > > > > > > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > > > > <melanieplageman@gmail.com> 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/
On Tue, Mar 15, 2022 at 10:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Tue, Mar 15, 2022 at 3:34 AM Melanie Plageman > <melanieplageman@gmail.com> wrote: > > > > On Mon, Mar 14, 2022 at 4:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman > > > <melanieplageman@gmail.com> wrote: > > > > > > > > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund <andres@anarazel.de> wrote: > > > > > > > > > > Hi, > > > > > > > > > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > > > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > > > > > <melanieplageman@gmail.com> 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.
On Wed, Mar 16, 2022 at 8:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Mar 15, 2022 at 10:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Tue, Mar 15, 2022 at 3:34 AM Melanie Plageman > > <melanieplageman@gmail.com> wrote: > > > > > > On Mon, Mar 14, 2022 at 4:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman > > > > <melanieplageman@gmail.com> wrote: > > > > > > > > > > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund <andres@anarazel.de> wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote: > > > > > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman > > > > > > > <melanieplageman@gmail.com> 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.enterprisedb.com/
Hi,
On Wed, Mar 16, 2022 at 11:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Mar 16, 2022 at 8:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Mar 15, 2022 at 10:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Tue, Mar 15, 2022 at 3:34 AM Melanie Plageman
> > > <melanieplageman@gmail.com> wrote:
> > > >
> > > > On Mon, Mar 14, 2022 at 4:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > >
> > > > > On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman
> > > > > <melanieplageman@gmail.com> wrote:
> > > > > >
> > > > > > On Sat, Mar 12, 2022 at 3:15 PM Andres Freund <andres@anarazel.de> wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote:
> > > > > > > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman
> > > > > > > > <melanieplageman@gmail.com> 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.
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.
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.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On Fri, Jul 1, 2022 at 7:12 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Mar 16, 2022 at 11:34 PM Masahiko Sawada <sawada.mshk@gmail.com> 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.
On Fri, Jul 1, 2022 at 3:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jul 1, 2022 at 7:12 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Mar 16, 2022 at 11:34 PM Masahiko Sawada <sawada.mshk@gmail.com> 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/
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
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
On Sat, Jul 2, 2022 at 2:53 Andres Freund <andres@anarazel.de> 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
On Sat, Jul 2, 2022 at 9:52 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Sat, Jul 2, 2022 at 2:53 Andres Freund <andres@anarazel.de> 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/
Attachment
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
On Wed, Jul 6, 2022 at 6:52 AM Andres Freund <andres@anarazel.de> 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/
Attachment
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?
On Wed, Jul 6, 2022 at 10:48 AM Andres Freund <andres@anarazel.de> 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/
Attachment
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
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.
On Thu, Jul 7, 2022 at 12:53 AM Andres Freund <andres@anarazel.de> 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/
Attachment
On Thu, Jul 7, 2022 at 1:28 AM Andres Freund <andres@anarazel.de> 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/
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?
On Tue, Jul 12, 2022 at 3:26 AM Andres Freund <andres@anarazel.de> 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.
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
Attachment
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
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!
On Fri, Oct 7, 2022 at 9:27 AM Andres Freund <andres@anarazel.de> 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
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