Hi, On 2022-07-01 10:41:55 +0900, Masahiko Sawada 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.
Why is it relevant where the stats are reported? > 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. It ensures that the stats are dropped if the subscription fails to be created partway through / the transaction is aborted. There's probably no way for that to happen today, but it still seems the right thing. > On the other hand, if we create the subscription stats > there we can resolve the issue Melanie reported in this thread. I am confused what the place of creation addresses? > 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. Yep, that needs to be updated. Greetings, Andres Freund