Re: Issue with pg_stat_subscription_stats

On Fri, Jul 1, 2022 at 3:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jul 1, 2022 at 7:12 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Mar 16, 2022 at 11:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> >
> > 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.
> >
>
> It won't create the stats entry in the shared hash table, so the
> behavior should be the same as without shared stats.

Yes, my point is that it may be misleading that the subscription stats
are created when a subscription is created. The initial behavior is
technically the same for pg_stat_database. That is, we don't create
the stats entry for them when creating the object. But we don’t call
pgstat_create_transactional when creating a database (we don’t have a
function like pgstat_create_database()) whereas we do for subscription
stats.

On the other hand, I'm not sure we agreed that the behavior that
Melanie reported is not a problem. The user might get confused since
the subscription stats works differently than other stats when a
reset. Previously, the primary reason why I hesitated to create the
subscription stats when creating a subscription is that CREATE
SUBSCRIPTION (with create_slot = false) can be rolled back. But with
the shmem stats, we can easily resolve it by using
pgstat_create_transactional().

>
> > 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.
> >
>
> +1.
>
> > IIUC autovacuum is no longer
> > responsible for garbage collection.
> >
>
> Right, this is my understanding as well.

Thank you for the confirmation.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/