Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.6\))
Subject: Re: Optionally automatically disable logical replication
 subscriptions on error
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: 
 <CAHut+Pv1Y0LtffP=3AnZFASwJJBGeZM7bi_vYON_hfyX=VdBJg@mail.gmail.com>
Date: Mon, 21 Jun 2021 19:49:56 -0700
Cc: Amit Kapila <amit.kapila16@gmail.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
 "Smith, Peter" <peters@fast.au.fujitsu.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <0646E5E0-EDF2-4D74-89DD-1C9BF0D7EA67@enterprisedb.com>
References: <DB35438F-9356-4841-89A0-412709EBD3AB@enterprisedb.com>
 <CAA4eK1KkhsNAW3=XxOdVRD9+RcQG5PRRFBFzAMibMG0YFFmAmg@mail.gmail.com>
 <B522A0FA-0B5C-4A66-9B73-928CAA86FB88@enterprisedb.com>
 <CAA4eK1KM23D06_rUEsgnTFUpABoeCCoTun2TMrQzWgxWOYY7fA@mail.gmail.com>
 <DE7E13B7-DC76-416A-A98F-3BC3F80E6BE9@enterprisedb.com>
 <CAD21AoB+QH4or-QDiakALZp6pkXiAaGxfGcsvNYJoDHk9kknZw@mail.gmail.com>
 <F25752DE-45EC-4DDE-97D9-75F270FB67CD@enterprisedb.com>
 <CAA4eK1+NoRbYSH1J08zi4OJ_EUMcjmxTwnmwVqZ6e_xzS0D6VA@mail.gmail.com>
 <14738A39-8000-463E-A0BA-6729930A32DA@enterprisedb.com>
 <CAA4eK1+TT7Sp2VTFAcFR+wrCWiZCi1rSC5HnffMgYAg4pdxEsw@mail.gmail.com>
 <A539C848-670E-454F-B31C-82D3CBE9F5AC@enterprisedb.com>
 <CAA4eK1K_JFSFrAkr_fgp3VX6hTSmjK=wNs4Tw8rUWHGp0+Bsaw@mail.gmail.com>
 <CAA4eK1+2hsBPVn_WD6hNMZe4hWPjxshP_OegF_Py5aEmJWrWEg@mail.gmail.com>
 <CAHut+Pv1Y0LtffP=3AnZFASwJJBGeZM7bi_vYON_hfyX=VdBJg@mail.gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/0646E5E0-EDF2-4D74-89DD-1C9BF0D7EA67%40enterprisedb.com>
Precedence: bulk


> On Jun 21, 2021, at 5:57 PM, Peter Smith <smithpb2250@gmail.com> =
wrote:
>=20
> * Is the goal to prevent some *unattended* SUBSCRIPTION from going bad
> at some point in future and then going into a relaunch loop for
> days/weeks and causing 1000's of errors without the user noticing. In
> that case, this patch seems to be quite useful, but for this goal
> maybe you don't want to be checking the tablesync workers at all, but
> should only be checking the apply worker like your original v1 patch
> did.

Yeah, my motivation was preventing an infinite loop, and providing a =
clean way for the users to know that replication they are waiting for =
won't ever complete, rather than having to infer that it will never =
halt.=20

> * Is the goal just to be a convenient way to disable the subscription
> during the CREATE SUBSCRIPTION phase so that the user can make
> corrections in peace without the workers re-launching and making more
> error logs?

No.  This is not and never was my motivation.  It's an interesting =
question, but that idea never crossed my mind.  I'm not sure what =
changes somebody would want to make *after* creating the subscription.  =
Certainly, there may be problems with how they have things set up, but =
they won't know that until the first error happens.

> Here the patch is helpful, but only for simple scenarios
> like 1 faulty table. Imagine if there are 10 tables (all with PK
> violations at DATASYNC copy) then you will encounter them one at a
> time and have to re-enable the subscription 10 times, after fixing
> each error in turn.

You are assuming disable_on_error=3Dtrue.  It is false by default.  But =
ok, let's accept that assumption for the sake of argument.  Now, will =
you have to manually go through the process 10 times?  I'm not sure.  =
The user might figure out their mistake after seeing the first error.

> So in this scenario the new option might be more
> of a hindrance than a help because it would be easier if the user just
> did "ALTER SUBSCRIPTION sub DISABLE" manually and fixed all the
> problems in one sitting before re-enabling.

Yeah, but since the new option is off by default, I don't see any =
sensible complaint.

>=20
> * etc
>=20
> //////////
>=20
> Finally, here is one last (crazy?) thought-bubble just for
> consideration. I might be wrong, but my gut feeling is that the Stats
> Collector is intended more for "tracking" and for "metrics" rather
> than for holding duplicates of logged error messages. At the same
> time, I felt that disabling an entire subscription due to a single
> rogue error might be overkill sometimes.

I'm happy to entertain criticism of the particulars of how my patch =
approaches this problem, but it is already making a distinction between =
transient errors (resources, network, etc.) vs. ones that are =
non-transient.  Again, I might not have drawn the line in the right =
place, but the patch is not intended to disable subscriptions in =
response to transient errors.

> But I wonder if there is a
> way to combine those two ideas so that the Stats Collector gets some
> new counter for tracking the number of worker re-launches that have
> occurred, meanwhile there could be a subscription option which gives a
> threshold above which you would disable the subscription.
> e.g.
> "disable_on_error_threshold=3D0" default, relaunch forever
> "disable_on_error_threshold=3D1" disable upon first error encountered.
> (This is how your patch behaves now I think.)
> "disable_on_error_threshold=3D500" disable if the re-launch errors go
> unattended and happen 500 times.

That sounds like a misfeature to me.  You could have a subscription that =
works fine for a month, surviving numerous short network outages, but =
then gets autodisabled after a longer network outage.  I'm not sure why =
anybody would want that.  You might argue for exponential backoff, where =
it never gets autodisabled on transient errors, but retries less =
frequently.  But I don't want to expand the scope of this patch to =
include that, at least not without a lot more evidence that it is =
needed.

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company