Re: Issue with pg_stat_subscription_stats - Mailing list pgsql-hackers

  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/