public inbox for [email protected]
help / color / mirror / Atom feedFrom: Greg Nancarrow <[email protected]>
To: [email protected] <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Mark Dilger <[email protected]>
Cc: Smith, Peter <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Optionally automatically disable logical replication subscriptions on error
Date: Mon, 6 Dec 2021 15:16:18 +1100
Message-ID: <CAJcOf-dkEVtRstWMchEAjb02xp_5cg_5g_0GUCthUx+o-bYdXg@mail.gmail.com> (raw)
In-Reply-To: <TYCPR01MB83732011D83A027C32A2802CED6A9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
References: <[email protected]>
<CAA4eK1KkhsNAW3=XxOdVRD9+RcQG5PRRFBFzAMibMG0YFFmAmg@mail.gmail.com>
<[email protected]>
<CAA4eK1KM23D06_rUEsgnTFUpABoeCCoTun2TMrQzWgxWOYY7fA@mail.gmail.com>
<[email protected]>
<CAD21AoB+QH4or-QDiakALZp6pkXiAaGxfGcsvNYJoDHk9kknZw@mail.gmail.com>
<[email protected]>
<CAD21AoCPSU-MBHkortBnELO7Hu0tLsX5GoOEbSYzyxKBPkKvBg@mail.gmail.com>
<TYCPR01MB837384DA6C879A709EBF30D1ED8B9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CALDaNm0bXsyrXg3dQGbAhkUDGKnFXtM6mKk8d-8VFLo6wz3ZJA@mail.gmail.com>
<TYCPR01MB8373934498C93D1A1466A2E3ED939@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CAJcOf-ci4NeNwowE=BmtSWip2NRgZdGe-VFjvSDY2JP0NNLTMQ@mail.gmail.com>
<TYCPR01MB8373D8A622ED51CDEF3DAF5DED949@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CALDaNm0PdzqVSy0uUZ+UKd9rQeQONUjkALj3Ge+fAmdw0cSLgA@mail.gmail.com>
<TYCPR01MB8373771371B31E1E6CC74B0AED999@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CAJcOf-f6oqtKt2CD-KcUH00=3+Bybym7magDenegOao2YE-SZQ@mail.gmail.com>
<TYCPR01MB837308EA9546476CFA0C53F0ED9B9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CALDaNm2g8E-cF+6DzzMe8zdeCt6Ktd454_1UFBRA+2jE3si3Zg@mail.gmail.com>
<TYCPR01MB837398F781D4B21A94B4EDE0ED639@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CAJcOf-c6TC37C7w4JKFDLxs3XaQO3tqhW3792T-_tv-a7hi5uA@mail.gmail.com>
<TYCPR01MB83735AA021E0F614A3AB3221ED679@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CALDaNm309nOxCtX0SOwHdUUp7LkrR9AYyjggGhy0zv1=2FWoiQ@mail.gmail.com>
<TYCPR01MB8373369C42D82D8F85E4AA83ED689@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CAA4eK1+VtBf4QC7CCd91Fh=VD_aXt83dGVVEP64YyBubEW1kVQ@mail.gmail.com>
<TYCPR01MB83730B37108684983C8F690DED699@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CAA4eK1+VS6LiD844jOArYTXTgWfiDVUmUmbP_1gG-PvYXp_JEA@mail.gmail.com>
<TYCPR01MB83732944B402BAE715D5E8F6ED699@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<TYCPR01MB83732011D83A027C32A2802CED6A9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
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:
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
view thread (74+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Optionally automatically disable logical replication subscriptions on error
In-Reply-To: <CAJcOf-dkEVtRstWMchEAjb02xp_5cg_5g_0GUCthUx+o-bYdXg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox