Re: Excessive number of replication slots for 12->14 logical replication

On Thu, Aug 18, 2022 at 12:44 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Thu, Aug 18, 2022 at 12:40 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Thu, Aug 18, 2022 at 11:38 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Aug 18, 2022 at 10:50 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > >
> > > > Ajin Cherian, do you want to update the patch according to these
> > > > comments? or shall I?
> > > >
> > > > Regards,
> > > >
> > >
> > > I will create a patch for this in a short while.
> >
> > Great, thanks!
> >
> > Regards,
> >
>
> Attached a patch with the changes.

Thank you for updating the patch!

> In DropSubscription(), I have let
> GetSubscriptionRelations()
> filter the READY state relations but added a check for !SYNCDONE state
> while cleaning up origin
> tracking. This matches with the logic used for clean-up of tablesync
> slots below in the same function.

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.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/