Re: Issue with pg_stat_subscription_stats - Mailing list pgsql-hackers

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/