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

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.

-- 
With Regards,
Amit Kapila.