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

On Fri, Aug 12, 2022 at 12:44 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Fri, Aug 12, 2022 at 11:28 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Fri, Aug 12, 2022 at 12:04 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Aug 4, 2022 at 5:42 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > > Thank you for working on this. I have a comment and a question:
> > > >
> > > > * This has to be done after updating the state
> > > > because otherwise if
> > > > * there is an error while doing the database
> > > > operations we won't be
> > > > - * able to rollback dropped slot.
> > > > + * able to rollback dropped slot or origin tracking.
> > > >
> > > > I think we can actually roll back dropping the replication origin. So
> > > > the above comment is true for only replication slots.
> > > >
> > >
> > > Fixed this.
> >
> > Thank you for updating the patch.
> >
> > /*
> > - * Cleanup the tablesync slot.
> > + * Cleanup the origin tracking and tablesync slot.
> > *
> > * This has to be done after updating the state
> > because otherwise if
> > * there is an error while doing the database
> > operations we won't be
> > * able to rollback dropped slot.
> > */
> >
> > The first paragraph mentioned the cleanup of both tablesync slot and
> > origin, but the second paragraph mentioned only the tablesync slot
> > despite "this" probably meaning the cleanup of both. I think that we
> > can just add the comment for dropping the origin while not touching
> > the comment for dropping the slot.
> >
> > The rest looks good to me.
> >
>
> Updated.

Thank you for updating the patch.

/*
- * Cleanup the tablesync slot.
+ * Cleanup the origin tracking and tablesync slot.
*
* This has to be done after updating the state because otherwise if
* there is an error while doing the database operations we won't be
- * able to rollback dropped slot.
+ * able to rollback the dropped slot. Origin tracking is dropped before
+ * the tablesync slot is dropped.
*/

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.

Regards,

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