Re: Optionally automatically disable logical replication subscriptions on error

On Sat, Dec 4, 2021 at 12:20 AM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)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