On Tue, Aug 23, 2022 at 7:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Aug 18, 2022 at 11:28 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Thu, Aug 18, 2022 at 3:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > Looks good.
> > >
> > > I have one minor comment:
> > >
> > > - * SUBREL_STATE_FINISHEDCOPY. The apply worker can also
> > > - * concurrently try to drop the origin and by this time
> > > - * the origin might be already removed. For these reasons,
> > > - * passing missing_ok = true.
> > > + * SUBREL_STATE_FINISHEDCOPY. So passing missing_ok = true.
> > >
> > > I think we should change "the apply worker" to "the tablesync worker"
> > > but should not remove this sentence. The fact that another process
> > > could concurrently try to drop the origin is still true.
> > >
> > > The rest looks good to me.
> > >
> >
> > Updated as described.
> >
>
> The patch looks good to me though I would like to test it a bit more
> before pushing.
>
While testing/reviewing it further, I noticed that the patch has used
missing_ok as true when dropping origin via tablesync worker. I don't
think that is correct because the concurrent operations that remove
origin like a refresh for the subscription take an access exclusive
lock on pg_subscription which prevent the previous operation to update
the rel state to SUBREL_STATE_SYNCDONE to succeed. So, I think we
should pass missing_ok as false which would be consistent with slot
handling. I have changed that and comments a few places. What do you
think of the attached?
--
With Regards,
Amit Kapila.