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

On Monday, September 12, 2022 2:15 PM Amit Kapila <amit.kapila16@gmail.com>
> 
> 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.

Thanks! The comments and changes look good to me.

Best regards,
Hou zj