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: 
 <CAA4eK1KM23D06_rUEsgnTFUpABoeCCoTun2TMrQzWgxWOYY7fA@mail.gmail.com>
Date: Sat, 19 Jun 2021 07:44:11 -0700
Cc: Sawada Masahiko <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: <DE7E13B7-DC76-416A-A98F-3BC3F80E6BE9@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>
To: Amit Kapila <amit.kapila16@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/DE7E13B7-DC76-416A-A98F-3BC3F80E6BE9%40enterprisedb.com>
Precedence: bulk


> On Jun 19, 2021, at 3:17 AM, Amit Kapila <amit.kapila16@gmail.com> =
wrote:
>=20
> Right, but there are things that could be common from the design
> perspective.

I went to reconcile my patch with that from [1] only to discover there =
is no patch on that thread.  Is there one in progress that I can see?

I don't mind trying to reconcile this patch with what you're discussing =
in [1], but I am a bit skeptical about [1] becoming a reality and I =
don't want to entirely hitch this patch to that effort.  This can be =
committed with or without any solution to the idea in [1].  The original =
motivation for this patch was that the TAP tests don't have a great way =
to deal with a subscription getting into a fail-retry infinite loop, =
which makes it harder for me to make progress on [2].  That doesn't =
absolve me of the responsibility of making this patch a good one, but it =
does motivate me to keep it simple.

> For example, why is it mandatory to update this conflict
> ( error) information in the system catalog instead of displaying it
> via some stats view?

The catalog must be updated to disable the subscription, so placing the =
error information in the same row doesn't require any independent =
touching of the catalogs.  Likewise, the catalog must be updated to =
re-enable the subscription, so clearing the error from that same row =
doesn't require any independent touching of the catalogs.

The error information does not *need* to be included in the catalog, but =
placing the information in any location that won't survive server =
restart leaves the user no information about why the subscription got =
disabled after a restart (or crash + restart) happens.

Furthermore, since v2 removed the "disabled_by_error" field in favor of =
just using subenabled + suberrmsg to determine if the subscription was =
automatically disabled, not having the information in the catalog would =
make it ambiguous whether the subscription was manually or automatically =
disabled.

> Also, why not also log the xid of the failed
> transaction?

We could also do that.  Reading [1], it seems you are overly focused on =
user-facing xids.  The errdetail in the examples I've been using for =
testing, and the one mentioned in [1], contain information about the =
conflicting data.  I think users are more likely to understand that a =
particular primary key value cannot be replicated because it is not =
unique than to understand that a particular xid cannot be replicated.  =
(Likewise for permissions errors.)  For example:

2021-06-18 16:25:20.139 PDT [56926] ERROR:  duplicate key value violates =
unique constraint "s1_tbl_unique"
2021-06-18 16:25:20.139 PDT [56926] DETAIL:  Key (i)=3D(1) already =
exists.
2021-06-18 16:25:20.139 PDT [56926] CONTEXT:  COPY tbl, line 2=20

This tells the user what they need to clean up before they can continue. =
 Telling them which xid tried to apply the change, but not the change =
itself or the conflict itself, seems rather unhelpful.  So at best, the =
xid is an additional piece of information.  I'd rather have both the =
ERROR and DETAIL fields above and not the xid than have the xid and lack =
one of those two fields.  Even so, I have not yet included the DETAIL =
field because I didn't want to bloat the catalog.

For the problem in [1], having the xid is more important than it is in =
my patch, because the user is expected in [1] to use the xid as a =
handle.  But that seems like an odd interface to me.  Imagine that a =
transaction on the publisher side inserted a batch of data, and only a =
subset of that data conflicts on the subscriber side.  What advantage is =
there in skipping the entire transaction?  Wouldn't the user rather skip =
just the problematic rows?  I understand that on the subscriber side it =
is difficult to do so, but if you are going to implement this sort of =
thing, it makes more sense to allow the user to filter out data that is =
problematic rather than filtering out xids that are problematic, and the =
filter shouldn't just be an in-or-out filter, but rather a mapping =
function that can redirect the data someplace else or rewrite it before =
inserting or change the pre-existing conflicting data prior to applying =
the problematic data or whatever.  That's a huge effort, of course, but =
if the idea in [1] goes in that direction, I don't want my patch to have =
already added an xid field which ultimately nobody wants.

[1] - =
https://www.postgresql.org/message-id/CAD21AoDeScrsHhLyEPYqN3sydg6PxAPVBbo=
K%3D30xJfUVihNZDA%40mail.gmail.com

[2] - =
https://www.postgresql.org/message-id/flat/915B995D-1D79-4E0A-BD8D-3B26792=
5FCE9%40enterprisedb.com#dbbce39c9e460183b67ee44b647b1209
=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company