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

On Mon, Sep 12, 2022 at 3:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Sep 12, 2022 at 10:22 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> >
> > Thank you for the patch!
> >
> > I agree with the approach of the patch to fix this issue. And I've
> > confirmed the issue doesn't happen with this patch. Here are some
> > review comments:
> >
> >                 /*
> >                  * UpdateSubscriptionRelState must be called within a
> > transaction.
> > -                * That transaction will be ended within the
> > finish_sync_worker().
> >                  */
> >
> > I think we can move the removed sentence to where we added
> > StartTransactionCommand(). For example,
> >
> > * Start a new transaction to cleanup the tablesync origin tracking.
> > * That transaction will be ended within the finish_sync_worker().
> >
> > ---
> >         * This has to be done after the data changes because otherwise if
> >
> > I think we can change this sentence back to the one we had before:
> >
> >         * This has to be done after updating the state because otherwise if
> >
>
> Changed as per suggestions.
>
> > ---
> > +       CommitTransactionCommand();
> > +
> >
> > We need to do pgstat_report_stat() since we performed DML.
> >
>
> Right, so called pgstat_report_stat() with 'force' as false because we
> will anyway do that later in finish_sync_worker().
>
> > ---
> > +       /*
> > +        * Start a new transaction to cleanup the tablesync origin tracking.
> > +        *
> > +        * We need to do this after the table state is set to SYNCDONE,
> > +        * otherwise if an error occurs while performing the database
> > +        * operation, the worker will be restarted, but the in-memory
> > +        * replication progress(remote_lsn) has been cleaned and will not be
> > +        * rolledback, so the restarted worker will use invalid replication
> > +        * progress resulting in replay of transactions that have already been
> > +        * applied.
> > +        */
> >
> > How about mentioning that even if the tablesync worker failed to drop
> > the replication origin, the tablesync worker won't restart but the
> > apply worker will do that?
> >
>
> Changed this and a few other comments in the patch. Kindly let me know
> what you think of the attached.

Thank you for updating the patch. It looks good to me.

Regards,

-- 
Masahiko Sawada