On Monday, December 6, 2021 1:16 PM Greg Nancarrow <[email protected]> wrote: > On Sat, Dec 4, 2021 at 12:20 AM [email protected] > <[email protected]> wrote: > > > > Hi, I've made a new patch v11 that incorporated suggestions described > above. > > > > Some review comments for the v11 patch: Thank you for your reviews ! > doc/src/sgml/ref/create_subscription.sgml > (1) Possible wording improvement? > > BEFORE: > + Specifies whether the subscription should be automatically disabled > + if replicating data from the publisher triggers errors. The default > + is <literal>false</literal>. > AFTER: > + Specifies whether the subscription should be automatically disabled > + if any errors are detected by subscription workers during data > + replication from the publisher. The default is <literal>false</literal>. Fixed.
> src/backend/replication/logical/worker.c
> (2) WorkerErrorRecovery comments
> Instead of:
>
> + * As a preparation for disabling the subscription, emit the error,
> + * handle the transaction and clean up the memory context of
> + * error. ErrorContext is reset by FlushErrorState.
>
> why not just say:
>
> + Worker error recovery processing, in preparation for disabling the
> + subscription.
>
> And then comment the function's code lines:
>
> e.g.
>
> /* Emit the error */
> ...
> /* Abort any active transaction */
> ...
> /* Reset the ErrorContext */
> ...
Agreed. Fixed.
> (3) DisableSubscriptionOnError return
>
> The "if (!subform->subdisableonerr)" block should probably first:
> heap_freetuple(tup);
>
> (regardless of the fact the only current caller will proc_exit anyway)
Fixed.
> (4) did_error flag
>
> I think perhaps the previously-used flag name "disable_subscription"
> is better, or maybe "error_recovery_done".
> Also, I think it would look better if it was set AFTER
> WorkerErrorRecovery() was called.
Adopted error_recovery_done
and changed its places accordingly.
> (5) DisableSubscriptionOnError LOG message
>
> This version of the patch removes the LOG message:
>
> + ereport(LOG,
> + errmsg("logical replication subscription \"%s\" will be disabled due
> to error: %s",
> + MySubscription->name, edata->message));
>
> Perhaps a similar error message could be logged prior to EmitErrorReport()?
>
> e.g.
> "logical replication subscription \"%s\" will be disabled due to an error"
Added.
I've attached the new version v12.
Best Regards,
Takamichi Osumi
v12-0001-Optionally-disable-subscriptions-on-error.patch
Description: v12-0001-Optionally-disable-subscriptions-on-error.patch
