On Monday, December 6, 2021 1:16 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > On Sat, Dec 4, 2021 at 12:20 AM osumi.takami...@fujitsu.com > <osumi.takami...@fujitsu.com> 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