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

On Monday, September 12, 2022 2:15 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>
> On Mon, Sep 12, 2022 at 10:22 AM Masahiko Sawada
> <sawada(dot)mshk(at)gmail(dot)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