Re: Issue with pg_stat_subscription_stats

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/