On Wed, Aug 24, 2022 at 3:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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?
>
Yes, this makes sense. I tested with a debugger to create simultaneous
alter subscription refresh publication AND tablesync worker
deleting the tracking origin. What I see is that the alter subscription command
gets the lock on SubscriptionRelRelationId and then actually goes on to
kill the tablesync worker. Even if the tablesync worker is waiting for
the same lock,
it looks like the SIGTERM is being handled and the worker is killed.
From my testing, there doesn't seem to be a case where the tablesync worker
would try and drop a previously dropped origin. So, I think we can pass
missing_ok as 'false' while dropping origin. The patch looks good to me.
regards,
Ajin Cherian
Fujitsu Australia