Re: Issue with pg_stat_subscription_stats

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