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

On Tue, Sep 5, 2023 at 11:32 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Monday, September 4, 2023 10:42 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> Hi,
>
> > On Mon, Sep 4, 2023 at 9:38 PM Zhijie Hou (Fujitsu) <houzj(dot)fnst(at)fujitsu(dot)com>
> > wrote:
> > >
> > > On Wednesday, July 5, 2023 2:53 PM Masahiko Sawada
> > <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > > Thanks for reviewing the patch. Pushed.
> > >
> > > My colleague Vignesh noticed that the newly added test cases were failing in
> > BF animal sungazer[1].
> >
> > Thank you for reporting!
> >
> > >
> > > The test failed to drop the slot which is active on publisher.
> > >
> > > --error-log--
> > > This failure is because pg_drop_replication_slot fails with "replication slot
> > "test_tab2_sub" is active for PID 55771638":
> > > 2023-09-02 09:00:04.806 UTC [12910732:4] 026_stats.pl LOG: statement:
> > > SELECT pg_drop_replication_slot('test_tab2_sub')
> > > 2023-09-02 09:00:04.807 UTC [12910732:5] 026_stats.pl ERROR:
> > > replication slot "test_tab2_sub" is active for PID 55771638
> > > 2023-09-02 09:00:04.807 UTC [12910732:6] 026_stats.pl STATEMENT:
> > > SELECT pg_drop_replication_slot('test_tab2_sub')
> > > -------------
> > >
> > > I the reason is that the test DISABLEd the subscription before
> > > dropping the slot, while "ALTER SUBSCRIPTION DISABLE" doesn't wait for
> > > the walsender to release the slot, so it's possible that the walsender
> > > is still alive when calling
> > > pg_drop_replication_slot() to drop the slot on
> > > publisher(pg_drop_xxxslot() doesn't wait for slot to be released).
> >
> > I agree with your analysis.
> >
> > >
> > > I think we can wait for the slot to become inactive before dropping like:
> > > $node_primary->poll_query_until('otherdb',
> > > "SELECT NOT EXISTS (SELECT 1 FROM pg_replication_slots
> > WHERE active_pid IS NOT NULL)"
> > > )
> > >
> >
> > I prefer this approach but it would be better to specify the slot name in the
> > query.
> >
> > > Or we can just don't drop the slot as it’s the last testcase.
> >
> > Since we might add other tests after that in the future, I think it's better to drop
> > the replication slot (and subscription).
> >
> > >
> > > Another thing might be worth considering is we can add one parameter
> > > in
> > > pg_drop_replication_slot() to let user control whether to wait or not,
> > > and the test can be fixed as well by passing nowait=false to the func.
> >
> > While it could be useful in general we cannot use this approach for this issue
> > since it cannot be backpatched to older branches. We might want to discuss it
> > on a new thread.
> >
> > I've attached a draft patch to fix this issue. Feedback is very welcome.
>
> Thanks, it looks good to me.

Thank you for reviewing the patch.

I'll push the attached patch to master, v16, and v15 tomorrow, barring
any objections.

Regards,

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