Re: Issue with pg_stat_subscription_stats - Mailing list pgsql-hackers

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.