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:
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>.
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 */
...
(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)
(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.
(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"
Regards,
Greg Nancarrow
Fujitsu Australia
