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