Re: Subscription statistics are not dropped at DROP SUBSCRIPTION in some cases

On Thu, May 11, 2023 at 5:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, May 10, 2023 at 8:58 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> >
> > Hi,
> >
> > Masahiko Sawada <sawada.mshk@gmail.com>, 8 May 2023 Pzt, 10:24 tarihinde şunu yazdı:
> >>
> >> I've attached the patch. Feedback is very welcome.
> >
> >
> > Thanks for the patch, nice catch.
> > I can confirm that the issue exists on HEAD and gets resolved by this patch. Also it looks like stats are really
notaffected if transaction fails for some reason, as you explained. 
> > IMO, the patch will be OK after commit message is added.
>
> Thank you for reviewing the patch. I'll push the patch early next
> week, barring any objections.

After thinking more about it, I realized that this is not a problem
specific to HEAD. ISTM the problem is that by commit 7b64e4b3, we drop
the stats entry of subscription that is not associated with a
replication slot for apply worker, but we missed the case where the
subscription is not associated with both replication slots for apply
and tablesync. So IIUC we should backpatch it down to 15.

Since in pg15, since we don't create the subscription stats at CREATE
SUBSCRIPTION time but do when the first error is reported, we cannot
rely on the regression test suite. Also, to check if the subscription
stats is surely removed, using pg_stat_have_stats() is clearer. So I
added a test case to TAP tests (026_stats.pl).

On Tue, May 9, 2023 at 1:51 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> A comment:
>
> ```
> +       /*
> +        * Tell the cumulative stats system that the subscription is getting
> +        * dropped.
> +        */
> +       pgstat_drop_subscription(subid);
> ```
>
> Isn't it better to write down something you said as comment? Or is it quite trivial?
>
> > There is a chance the
> > transaction dropping the subscription fails due to network error etc
> > but we don't need to worry about it as reporting the subscription drop
> > is transactional.

I'm not sure it's worth mentioning as we don't have such a comment
around other pgstat_drop_XXX functions.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com