On Sat, Jul 2, 2022 at 9:52 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > On Sat, Jul 2, 2022 at 2:53 Andres Freund <and...@anarazel.de> wrote: >> >> Hi, >> >> On 2022-07-01 16:08:48 +0900, Masahiko Sawada wrote: >> > Yes, my point is that it may be misleading that the subscription stats >> > are created when a subscription is created. >> >> I think it's important to create stats at that time, because otherwise it's >> basically impossible to ensure that stats are dropped when a transaction >> rolls >> back. If some / all columns should return something else before stats are >> reported that can be addressed easily by tracking that in a separate field. >> >> >> > 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(). >> >> Yep. >> >> >> > > > 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. >> >> Want to propose a patch? > > > Yes, I’ll propose a patch. >
I've attached the patch, fix_drop_subscriptions_stats.patch, to fix it. I've also attached another PoC patch, poc_create_subscription_stats.patch, to create the stats entry when creating the subscription, which address the issue reported in this thread; the pg_stat_reset_subscription_stats() doesn't update the stats_reset if no error is reported yet. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
fix_drop_subscription_stats.patch
Description: Binary data
poc_create_subscription_stats.patch
Description: Binary data
