On Wed, Aug 17, 2022 at 6:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.
Agreed.
Ajin Cherian, do you want to update the patch according to these
comments? or shall I?
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/