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.