Re: Issue with pg_stat_subscription_stats - Mailing list pgsql-hackers

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