On Wed, Aug 17, 2022 at 1:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Aug 16, 2022 at 8:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Aug 12, 2022 at 10:52 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Fri, Aug 12, 2022 at 12:44 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > >
> > >
> > > ISTM that the "This" in the first sentence in the second paragraph
> > > still indicates the cleanup of the origin tracking and table sync
> > > slot. How about not having the common comment for both like the patch
> > > I've attached? In addition to this change, I moved the code to drop
> > > the origin before stopping the streaming so that it can be close to
> > > the slot drop. But feel free to revert this change.
> > >
> >
> > After this change, won't the code in AlterSubscription_refresh()
> > related to origin drop [1] needs to be updated? Firstly, now even if
> > the state is SYNCDONE, we would have already dropped the origin. Then,
> > I think the race mentioned in the comments no longer remains true
> > because before dropping the origin in tablesync worker, we would have
> > taken a conflicting lock on pg_subscription_rel.
>
> Right.
>
> Probably the same is true for DropSubscription()?
>
Yes.
> That is, we can skip
> removing the origin if the state is SYNCDONE (READ-state relations are
> already filtered by GetSubscriptionRelations()).
>
I think we can pass the second parameter as false in
GetSubscriptionRelations() and then filter outside for both READY and
SYNCDONE states.
--
With Regards,
Amit Kapila.