public inbox for [email protected]
help / color / mirror / Atom feedFrom: [email protected] <[email protected]>
To: [email protected] <[email protected]>
To: [email protected] <[email protected]>
Cc: vignesh C <[email protected]>
Cc: 'Greg Nancarrow' <[email protected]>
Cc: Amit Kapila <[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: Thu, 6 Jan 2022 03:16:30 +0000
Message-ID: <OS0PR01MB61132035CF205C142FF6E2E2FB4C9@OS0PR01MB6113.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <TYCPR01MB83732A590139C59DE0A83CFBED4B9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
References: <TYCPR01MB83732011D83A027C32A2802CED6A9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CAJcOf-dkEVtRstWMchEAjb02xp_5cg_5g_0GUCthUx+o-bYdXg@mail.gmail.com>
<TYCPR01MB83737F77BBE63432611CE7C8ED6D9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CALDaNm1O8iBHSjfkv2-Ld6B7C4ay8F3-O1Q2QHVrM8H7Zu40iA@mail.gmail.com>
<TYCPR01MB8373DEBEB6EC28CB82F50D47ED759@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CAJcOf-fY7VspwzN2PPDQq1c1DpvhxHuDv4DWq+=zDWb_60u-rQ@mail.gmail.com>
<TYCPR01MB837344D0306FFCAAE280F498ED779@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<OS3PR01MB6275C137E5032EC46290DB319E439@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<TYCPR01MB83732A590139C59DE0A83CFBED4B9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
On Wednesday, January 5, 2022 8:53 PM [email protected]
<[email protected]> wrote:
>
> On Tuesday, December 28, 2021 11:53 AM Wang, Wei/王 威
> <[email protected]> wrote:
> > On Thursday, December 16, 2021 8:51 PM [email protected]
> > <[email protected]> wrote:
> > > Attached the updated patch v14.
> >
> > A comment to the timing of printing a log:
> Thank you for your review !
>
> > After the log[1] was printed, I altered subscription's option
> > (DISABLE_ON_ERROR) from true to false before invoking
> > DisableSubscriptionOnError to disable subscription. Subscription was not
> > disabled.
> > [1] "LOG: logical replication subscription "sub1" will be disabled due to an
> > error"
> >
> > I found this log is printed in function WorkerErrorRecovery:
> > + ereport(LOG,
> > + errmsg("logical replication subscription \"%s\" will
> > be disabled due to an error",
> > + MySubscription->name));
> > This log is printed here, but in DisableSubscriptionOnError, there is a check to
> > confirm subscription's disableonerr field. If disableonerr is found changed from
> > true to false in DisableSubscriptionOnError, subscription will not be disabled.
> >
> > In this case, "disable subscription" is printed, but subscription will not be
> > disabled actually.
> > I think it is a little confused to user, so what about moving this message after
> > the check which is mentioned above in DisableSubscriptionOnError?
> Makes sense. I moved the log print after
> the check of the necessity to disable the subscription.
>
> Also, I've scrutinized and refined the new TAP test as well for refactoring.
> As a result, I fixed wait_for_subscriptions()
> so that some extra codes that can be simplified,
> such as escaped variable and one part of WHERE clause, are removed.
> Other change I did is to replace two calls of wait_for_subscriptions()
> with polling_query_until() for the subscriber, in order to
> make the tests better and more suitable for the test purposes.
> Again, for this refinement, I've conducted a tight loop test
> to check no timing issue and found no problem.
>
Thanks for updating the patch. Here are some comments:
1)
+ /*
+ * We would not be here unless this subscription's disableonerr field was
+ * true when our worker began applying changes, but check whether that
+ * field has changed in the interim.
+ */
+ if (!subform->subdisableonerr)
+ {
+ /*
+ * Disabling the subscription has been done already. No need of
+ * additional work.
+ */
+ heap_freetuple(tup);
+ table_close(rel, RowExclusiveLock);
+ CommitTransactionCommand();
+ return;
+ }
I don't understand what does "Disabling the subscription has been done already"
mean, I think we only run here when subdisableonerr is changed in the interim.
Should we modify this comment? Or remove it because there are already some
explanations before.
2)
+ /* Set the subscription to disabled, and note the reason. */
+ values[Anum_pg_subscription_subenabled - 1] = BoolGetDatum(false);
+ replaces[Anum_pg_subscription_subenabled - 1] = true;
I didn't see the code corresponding to "note the reason". Should we modify the
comment?
3)
+ bool disableonerr; /* Whether errors automatically disable */
This comment is hard to understand. Maybe it can be changed to:
Indicates if the subscription should be automatically disabled when subscription
workers detect any errors.
Regards,
Tang
view thread (4+ 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], [email protected], [email protected]
Subject: RE: Optionally automatically disable logical replication subscriptions on error
In-Reply-To: <OS0PR01MB61132035CF205C142FF6E2E2FB4C9@OS0PR01MB6113.jpnprd01.prod.outlook.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