Issue with pg_stat_subscription_stats

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/

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
EDB: https://www.enterprisedb.com/

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/

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/

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?