Re: Issue with pg_stat_subscription_stats

On Sat, Jul 2, 2022 at 9:52 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
>
>
> On Sat, Jul 2, 2022 at 2:53 Andres Freund <andres(at)anarazel(dot)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/