Subject: Optionally automatically disable logical replication subscriptions on error

From: Mark Dilger <[email-masked]>
Date: Thu, 17 Jun 2021 13:18:38 -0700

Hackers,

Logical replication apply workers for a subscription can easily get stuck in an infinite loop of attempting to apply a change, triggering an error (such as a constraint violation), exiting with an error written to the subscription worker log, and restarting.

As things currently stand, only superusers can create subscriptions.  Ongoing work to delegate superuser tasks to non-superusers creates the potential for even more errors to be triggered, specifically, errors where the apply worker does not have permission to make changes to the target table.

The attached patch makes it possible to create a subscription using a new subscription_parameter, "disable_on_error", such that rather than going into an infinite loop, the apply worker will catch errors and automatically disable the subscription, breaking the loop.  The new parameter defaults to false.  When false, the PG_TRY/PG_CATCH overhead is avoided, so for subscriptions which do not use the feature, there shouldn't be any change.  Users can manually clear the error after fixing the underlying issue with an ALTER SUBSCRIPTION .. ENABLE command. 
 
In addition to helping on production systems, this makes writing TAP tests involving error conditions simpler.  I originally ran into the motivation to write this patch when frustrated that TAP tests needed to parse the apply worker log file to determine whether permission failures were occurring and what they were.  It was also obnoxiously easy to have a test get stuck waiting for a permanently stuck subscription to catch up.  This helps with both issues.

I don't think this is quite ready for commit, but I'd like feedback if folks like this idea or want to suggest design changes.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Amit Kapila <[email-masked]>
Date: Fri, 18 Jun 2021 10:17:36 +0530

> The attached patch makes it possible to create a subscription using a new subscription_parameter, "disable_on_error", such that rather than going into an infinite loop, the apply worker will catch…

I see this idea has merits and it will help users to repair failing
subscriptions. Few points on a quick look at the patch: (a) The patch
seem to be assuming that the error can happen only by the apply worker
but I think the constraint violation can happen via one of the table
sync workers as well, (b) What happens if the error happens when you
are updating the error information in the catalog table. I think
instead of seeing the actual apply time error, the user might see some
other for which it won't be clear what is an appropriate action.

We are also discussing another action like skipping the apply of the
transaction on an error [1]. I think it is better to evaluate both the
proposals as one seems to be an extension of another. Adding
Sawada-San, as he is working on the other proposal.

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

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Fri, 18 Jun 2021 16:34:47 +1000

> I don't think this is quite ready for commit, but I'd like feedback if folks like this idea or want to suggest design changes.

I tried your patch.

It applied OK (albeit with whitespace warnings).

The code build and TAP tests are all OK.

Below are a few comments and observations.

COMMENTS
========

(1) PG Docs catalogs.sgml

Documented new column "suberrmsg" but did not document the other new
columns ("disable_on_error", "disabled_by_error")?

------

(2) New column "disabled_by_error".

I wondered if there was actually any need for this column. Isn't the
same information conveyed by just having "subenabled" = false, at same
time as as non-empty "suberrmsg"? This would remove any confusion for
having 2 booleans which both indicate disabled.

------

(3) New columns "disabled_by_error", "disabled_on_error".

All other columns of the pg_subscription have a "sub" prefix.

------

(4) errhint member used?

@@ -91,12 +100,16 @@ typedef struct Subscription
  char    *name; /* Name of the subscription */
  Oid owner; /* Oid of the subscription owner */
  bool enabled; /* Indicates if the subscription is enabled */
+ bool disable_on_error; /* Whether errors automatically disable */
+ bool disabled_by_error; /* Whether an error has disabled */
  bool binary; /* Indicates if the subscription wants data in
  * binary format */
  bool stream; /* Allow streaming in-progress transactions. */
  char    *conninfo; /* Connection string to the publisher */
  char    *slotname; /* Name of the replication slot */
  char    *synccommit; /* Synchronous commit setting for worker */
+ char    *errmsg; /* Message from error which disabled */
+ char    *errhint; /* Hint from error which disabled */
  List    *publications; /* List of publication names to subscribe to */
 } Subscription;

I did not find any code using that newly added member "errhint".

------

(5) dump.c

i. No mention of new columns "disabled_on_error" and
"disabled_by_error". Is that right?

ii. Shouldn't the code for the "suberrmsg" be qualified with some PG
version number checks?

------

(6) Patch only handles errors only from the Apply worker.

Tablesync can give similar errors (e.g. PK violation during DATASYNC
phase) which will trigger re-launch forever regardless of the setting
of "disabled_on_error".
(confirmed by observations below)

------

(7) TAP test code

+$node_subscriber->init(allows_streaming => 'logical');

AFAIK that "logical" configuration is not necessary for the subscriber side. So,

$node_subscriber->init();

////////////

Some Experiments/Observations
==============================

In general, I found this functionality is useful and it works as
advertised by your patch comment.

======

Test: Display pg_subscription with the new columns
Observation: As expected. But some new colnames are not prefixed like
their peers.

test_sub=# \pset x
Expanded display is on.
test_sub=# select * from pg_subscription;
-[ RECORD 1 ]-----+--------------------------------------------------------
oid               | 16394
subdbid           | 16384
subname           | tap_sub
subowner          | 10
subenabled        | t
disable_on_error  | t
disabled_by_error | f
subbinary         | f
substream         | f
subconninfo       | host=localhost dbname=test_pub application_name=tap_sub
subslotname       | tap_sub
subsynccommit     | off
suberrmsg         |
subpublications   | {tap_pub}

======

Test: Cause a PK violation during normal Apply replication (when
"disabled_on_error=true")
Observation: Apply worker stops. Subscription is disabled. Error
message is in the catalog.

2021-06-18 15:12:45.905 AEST [25904] LOG:  edata is true for
subscription 'tap_sub': message = "duplicate key value violates unique
constraint "test_tab_pkey"", hint = "<NONE>"
2021-06-18 15:12:45.905 AEST [25904] LOG:  logical replication apply
worker for subscription "tap_sub" will stop because the subscription
was disabled due to error
2021-06-18 15:12:45.905 AEST [25904] ERROR:  duplicate key value
violates unique constraint "test_tab_pkey"
2021-06-18 15:12:45.905 AEST [25904] DETAIL:  Key (a)=(1) already exists.
2021-06-18 15:12:45.908 AEST [19924] LOG:  background worker "logical
replication worker" (PID 25904) exited with exit code 1

test_sub=# select * from pg_subscription;
-[ RECORD 1 ]-----+---------------------------------------------------------------
oid               | 16394
subdbid           | 16384
subname           | tap_sub
subowner          | 10
subenabled        | f
disable_on_error  | t
disabled_by_error | t
subbinary         | f
substream         | f
subconninfo       | host=localhost dbname=test_pub application_name=tap_sub
subslotname       | tap_sub
subsynccommit     | off
suberrmsg         | duplicate key value violates unique constraint
"test_tab_pkey"
subpublications   | {tap_pub}

======

Test: Try to enable subscription (without fixing the PK violation problem).
Observation. OK. It just stops again

test_sub=# alter subscription tap_sub enable;
ALTER SUBSCRIPTION
test_sub=# 2021-06-18 15:17:18.067 AEST [10228] LOG:  logical
replication apply worker for subscription "tap_sub" has started
2021-06-18 15:17:18.078 AEST [10228] LOG:  edata is true for
subscription 'tap_sub': message = "duplicate key value violates unique
constraint "test_tab_pkey"", hint = "<NONE>"
2021-06-18 15:17:18.078 AEST [10228] LOG:  logical replication apply
worker for subscription "tap_sub" will stop because the subscription
was disabled due to error
2021-06-18 15:17:18.078 AEST [10228] ERROR:  duplicate key value
violates unique constraint "test_tab_pkey"
2021-06-18 15:17:18.078 AEST [10228] DETAIL:  Key (a)=(1) already exists.
2021-06-18 15:17:18.079 AEST [19924] LOG:  background worker "logical
replication worker" (PID 10228) exited with exit code 1

======

Test: Manually disable the subscription (which had previously already
been disabled due to error)
Observation: OK. The suberrmsg gets reset to an empty string.

alter subscription tap_sub disable;

=====

Test: Turn off the disable_on_error
Observation: As expected, now the Apply worker goes into re-launch
forever loop every time it hits PK violation

test_sub=# alter subscription tap_sub set (disable_on_error=false);
ALTER SUBSCRIPTION

...

======

Test: Cause a PK violation in the Tablesync copy (DATASYNC) phase.
(when disable_on_error = true)
Observation: This patch changes nothing for this case. The Tablesyn
re-launchs in a forever loop the same as current functionality.

test_sub=# CREATE SUBSCRIPTION tap_sub CONNECTION 'host=localhost
dbname=test_pub application_name=tap_sub' PUBLICATION tap_pub WITH
(disable_on_error=false);
NOTICE:  created replication slot "tap_sub" on publisher
CREATE SUBSCRIPTION
test_sub=# 2021-06-18 15:38:19.547 AEST [18205] LOG:  logical
replication apply worker for subscription "tap_sub" has started
2021-06-18 15:38:19.557 AEST [18207] LOG:  logical replication table
synchronization worker for subscription "tap_sub", table "test_tab"
has started
2021-06-18 15:38:19.610 AEST [18207] ERROR:  duplicate key value
violates unique constraint "test_tab_pkey"
2021-06-18 15:38:19.610 AEST [18207] DETAIL:  Key (a)=(1) already exists.
2021-06-18 15:38:19.610 AEST [18207] CONTEXT:  COPY test_tab, line 1
2021-06-18 15:38:19.611 AEST [19924] LOG:  background worker "logical
replication worker" (PID 18207) exited with exit code 1
2021-06-18 15:38:24.634 AEST [18369] LOG:  logical replication table
synchronization worker for subscription "tap_sub", table "test_tab"
has started
2021-06-18 15:38:24.689 AEST [18369] ERROR:  duplicate key value
violates unique constraint "test_tab_pkey"
2021-06-18 15:38:24.689 AEST [18369] DETAIL:  Key (a)=(1) already exists.
2021-06-18 15:38:24.689 AEST [18369] CONTEXT:  COPY test_tab, line 1
2021-06-18 15:38:24.690 AEST [19924] LOG:  background worker "logical
replication worker" (PID 18369) exited with exit code 1
2021-06-18 15:38:29.701 AEST [18521] LOG:  logical replication table
synchronization worker for subscription "tap_sub", table "test_tab"
has started
2021-06-18 15:38:29.765 AEST [18521] ERROR:  duplicate key value
violates unique constraint "test_tab_pkey"
2021-06-18 15:38:29.765 AEST [18521] DETAIL:  Key (a)=(1) already exists.
2021-06-18 15:38:29.765 AEST [18521] CONTEXT:  COPY test_tab, line 1
2021-06-18 15:38:29.766 AEST [19924] LOG:  background worker "logical
replication worker" (PID 18521) exited with exit code 1
etc...

-[ RECORD 1 ]-----+--------------------------------------------------------
oid               | 16399
subdbid           | 16384
subname           | tap_sub
subowner          | 10
subenabled        | t
disable_on_error  | f
disabled_by_error | f
subbinary         | f
substream         | f
subconninfo       | host=localhost dbname=test_pub application_name=tap_sub
subslotname       | tap_sub
subsynccommit     | off
suberrmsg         |
subpublications   | {tap_pub}

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Mark Dilger <[email-masked]>
Date: Fri, 18 Jun 2021 12:36:28 -0700

> On Jun 17, 2021, at 9:47 PM, Amit Kapila <[email-masked]> wrote:
> 
> (a) The patch
> sync workers as well

You are right.  Peter mentioned the same thing, and it is clearly so.  I am working to repair this fault in v2 of the patch.

> (b) What happens if the error happens when you
> are updating the error information in the catalog table.

I think that is an entirely different kind of error.  The patch attempts to catch errors caused by the user, not by core functionality of the system failing.  If there is a fault that prevents the catalogs from being updated, it is unclear what the patch can do about that.

> I think

Good point.

Before trying to do much of anything with the caught error, the v2 patch logs the error.  If the subsequent efforts to disable the subscription fail, at least the logs should contain the initial failure message.  The v1 patch emitted a log message much further down, and really just intended for debugging the patch itself, with many opportunities for something else to throw before the log is written.

> proposals as one seems to be an extension of another.

Thanks for the link.

I think they are two separate options.  For some users and data patterns, subscriber-side skipping of specific problematic commits will be fine.  For other usage patterns, skipping earlier commits will results in more and more data integrity problems (foreign key references, etc.) such that the failures will snowball with skipping becoming the norm rather than the exception.  Users with those usage patterns would likely prefer the subscription to automatically be disabled until manual intervention can clean up the problem.

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Fri, 18 Jun 2021 17:03:45 -0700

> On Jun 17, 2021, at 11:34 PM, Peter Smith <[email-masked]> wrote:
> 
> I tried your patch.

Thanks for the quick and thorough review!

> having 2 booleans which both indicate disabled.

Yeah, I wondered about that before posting v1.  I removed the disabled_by_error field for v2.

> All other columns of the pg_subscription have a "sub" prefix.

I don't feel strongly about this.  How about "subdisableonerr"?  I used that in v2.

> I did not find any code using that newly added member "errhint".

Thanks for catching that.  I had tried to remove all references to "errhint" before posting v1.  The original idea was that both the message and hint of the error would be kept, but in testing I found the hint field was typically empty, so I removed it.  Sorry that I left one mention of it lying around.

> (5) dump.c

I didn't bother getting pg_dump working before posting v1, and I still have not done so, as I mainly want to solicit feedback on whether the basic direction I am going will work for the community.

> (confirmed by observations below)

Yes, this is a good point, and also mentioned by Amit.  I have fixed it in v2 and adjusted the regression test to trigger an automatic disabling for initial table sync as well as for change replication.

> constraint "test_tab_pkey"", hint = "<NONE>"

You didn't call this out, but FYI, I don't intend to leave this particular log message in the patch.  It was for development only.  I have removed it for v2 and have added a different log message much sooner after catching the error, to avoid squashing the error in case some other action fails.

The regression test shows this, if you open tmp_check/log/022_disable_on_error_subscriber.log:

2021-06-18 16:25:20.138 PDT [56926] LOG:  logical replication subscription "s1" will be disabled due to error: duplicate key value violates unique constraint "s1_tbl_unique"
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)=(1) already exists.
2021-06-18 16:25:20.139 PDT [56926] CONTEXT:  COPY tbl, line 2

The first line logs the error prior to attempting to disable the subscription, and the next three lines are due to rethrowing the error after committing the successful disabling of the subscription.  If the attempt to disable the subscription itself throws, these additional three lines won't show up, but the first one should.  Amit mentioned this upthread.  Do you think this will be ok, or would you like to also have a suberrdetail field so that the detail doesn't get lost?  I haven't added such an extra field, and am inclined to think it would be excessive, but maybe others feel differently?

> re-launchs in a forever loop the same as current functionality.

In v2, tablesync copy errors should also be caught.  The test has been extended to cover this also.

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Sat, 19 Jun 2021 15:47:18 +0530

On Sat, Jun 19, 2021 at 1:06 AM Mark Dilger
<[email-masked]> wrote:
> I think they are two separate options.
>

Right, but there are things that could be common from the design
perspective. For example, why is it mandatory to update this conflict
( error) information in the system catalog instead of displaying it
via some stats view? Also, why not also log the xid of the failed
transaction?

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Sat, 19 Jun 2021 07:44:11 -0700

> On Jun 19, 2021, at 3:17 AM, Amit Kapila <[email-masked]> wrote:
> 
> 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
> 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:

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.

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

---

From: Mark Dilger <[email-masked]>
Date: Sat, 19 Jun 2021 09:21:56 -0700

> On Jun 19, 2021, at 7:44 AM, Mark Dilger <[email-masked]> wrote:
> 
> 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.

Thinking about this some more, it seems my patch already sets the stage for this sort of thing.

We could extend the concept of triggers to something like ErrorTriggers that could be associated with subscriptions.  I already have the code catching errors for subscriptions where disable_on_error is true.  We could use that same code path for subscriptions that have one or more BEFORE or AFTER ErrorTriggers defined.  We could pass the trigger all the error context information along with the row and subscription information, and allow the trigger to either modify the data being replicated or make modifications to the table being changed.  I think having support for both BEFORE and AFTER would be important, as a common design pattern might be to move aside the conflicting rows in the BEFORE trigger, then reconcile and merge them back into the table in the AFTER trigger.  If the xid still cannot be replicated after one attempt using the triggers, the second attempt to disable the subscription instead.

There are a lot of details to consider, but to my mind this idea is much more user friendly than the idea that users should muck about with xids for arbitrarily many conflicting transactions.

—
Mark Dilger

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 21 Jun 2021 11:17:05 +0900

> 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 will submit the patch.

> 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.…

There was a discussion that the skipping transaction patch would also
need to have a feature that tells users the details of the last
failure transaction such as its XID, timestamp, action etc. In that
sense, those two patches might need the common infrastructure that the
apply workers leave the error details somewhere so that the users can
see it.

> 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…

Is it really useful to write only error message to the system catalog?
Even if we see the error message like "duplicate key value violates
unique constraint “test_tab_pkey”” on the system catalog, we will end
up needing to check the server log for details to properly resolve the
conflict. If the user wants to know whether the subscription is
disabled manually or automatically, the error message on the system
catalog might not necessarily be necessary.

> 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.  Imagin…

The feature discussed in that thread is meant to be a repair tool for
the subscription in emergency cases when something that should not
have happened happened. I guess that resolving row (or column) level
conflict should be done in another way, for example, by defining
policies for each type of conflict.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

---

From: Mark Dilger <[email-masked]>
Date: Sun, 20 Jun 2021 19:26:23 -0700

> On Jun 20, 2021, at 7:17 PM, Masahiko Sawada <[email-masked]> wrote:
> 
> I will submit the patch.

Great, thanks!

> apply workers leave the error details somewhere so that the users can

Right.  Subscription on error triggers would need that, too, if we wrote them.

> catalog might not necessarily be necessary.

We can put more information in there.  I don't feel strongly about it.  I'll wait for your patch to see what infrastructure you need.

> policies for each type of conflict.

I understand that is the idea, but I'm having trouble believing it will work that way in practice.  If somebody has a subscription that has gone awry, what reason do we have to believe there will only be one transaction that will need to be manually purged?  It seems just as likely that there would be a million transactions that need to be purged, and creating an interface for users to manually review them and keep or discard on a case by case basis seems unworkable.  Sure, you might have specific cases where the number of transactions to purge is small, but I don't like designing the feature around that assumption.

All the same, I'm looking forward to seeing your patch!

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Mon, 21 Jun 2021 08:23:00 +0530

> 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.…

I never said that we don't need the error information. I think we need
xid along with other things.

> 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 think skipping some changes but not others can make the final
transaction data inconsistent. Say, we have a case where, in a
transaction after insert, there is an update or delete on the same
row, then we might silently skip such updates/deletes unless the same
row is already present in the subscriber. I think skipping the entire
transaction based on user instruction would be safer than skipping
some changes that lead to an error.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 21 Jun 2021 08:39:34 +0530

> > catalog might not necessarily be necessary.

I think the two key points are (a) to define exactly what all
information is required to be logged on error, (b) where do we want to
store the information based on requirements. I see that for (b) Mark
is inclined to use the existing catalog table. I feel that is worth
considering but not sure if that is the best way to deal with it. For
example, if we store that information in the catalog, we might need to
consider storing it both in pg_subscription and pg_subscription_rel,
otherwise, we might overwrite the errors as I think what is happening
in the currently proposed patch. The other possibilities could be to
define a new catalog table to capture the error information or log the
required information via stats collector and then the user can see
that info via some stats view.

> I understand that is the idea, but I'm having trouble believing it will work that way in practice.  If somebody has a subscription that has gone awry, what reason do we have to believe there will only be one transaction that will need to be manually purged?
>

Because currently, we don't proceed after an error unless it is
resolved. Why do you think there could be multiple such transactions?

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 21 Jun 2021 12:50:31 +0900

On Mon, Jun 21, 2021 at 12:09 PM Amit Kapila <[email-masked]> wrote:
> information is required to be logged on error,

When it comes to the patch for skipping transactions, it would
somewhat depend on how users specify transactions to skip. On the
other hand, for this patch, the minimal information would be whether
the subscription is disabled automatically by the server.

> (b) where do we want to

This point is also related to the point whether or not that
information needs to last after the server crash (and restart). When
it comes to the patch for skipping transactions, there was a
discussion that we don’t necessarily need it since the tools will be
used in rare cases. But for this proposed patch, I guess it would be
useful if it does. It might be worth considering doing a different way
for each patch. For example, we send the details of last failure
transaction to the stats collector while updating subenabled to
something like “automatically-disabled” instead of to just “false” (or
using another column to show the subscriber is disabled automatically
by the server).

Regards,

---

From: Mark Dilger <[email-masked]>
Date: Sun, 20 Jun 2021 21:54:53 -0700

> On Jun 20, 2021, at 8:09 PM, Amit Kapila <[email-masked]> wrote:
> 

Just as one example, if the subscriber has a unique index that the publisher lacks, any number of transactions could add non-unique data that then fails to apply on the subscriber.  My patch took the view that the user should figure out how to get the subscriber side consistent with the publisher side, but if you instead take the approach that problematic commits should be skipped, it would seem that arbitrarily many such transactions could be committed on the publisher side.

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Mon, 21 Jun 2021 10:41:51 +0530

On Mon, Jun 21, 2021 at 10:24 AM Mark Dilger
<[email-masked]> wrote:
> Just as one example, if the subscriber has a unique index that the publisher lacks, any number of transactions could add non-unique data that then fails to apply on the subscriber.
>

Then also it will fail on the first such conflict, so even without
your patch, the apply worker corresponding to the subscription won't
be able to proceed after the first error, it won't lead to multiple
failing xids. However, I see a different case where there could be
multiple failing xids and that can happen during initial table sync
where multiple workers failed due to some error. I am not sure your
patch would be able to capture all such failed transactions because
you are recording this information in pg_subscription and not in
pg_subscription_rel.

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Sun, 20 Jun 2021 22:12:44 -0700

> (a) to define exactly what all
> store the information based on requirements.

I'm not sure it has to be stored anywhere durable.  I have a patch in the works to do something like:

create function foreign_key_insert_violation_before() returns conflict_trigger as $$
BEGIN
    RAISE NOTICE 'elevel: %', TG_ELEVEL:
    RAISE NOTICE 'sqlerrcode: %', TG_SQLERRCODE:
    RAISE NOTICE 'message: %', TG_MESSAGE:
    RAISE NOTICE 'detail: %', TG_DETAIL:
    RAISE NOTICE 'detail_log: %', TG_DETAIL_LOG:
    RAISE NOTICE 'hint: %', TG_HINT:
    RAISE NOTICE 'schema: %', TG_SCHEMA_NAME:
    RAISE NOTICE 'table: %', TG_TABLE_NAME:
    RAISE NOTICE 'column: %', TG_COLUMN_NAME:
    RAISE NOTICE 'datatype: %', TG_DATATYPE_NAME:
    RAISE NOTICE 'constraint: %', TG_CONSTRAINT_NAME:

        -- do something useful to prepare for retry of transaction
        -- which raised a foreign key violation
END
$$ language plpgsql;

create function foreign_key_insert_violation_after() returns conflict_trigger as $$
BEGIN
        -- do something useful to cleanup after retry of transaction
        -- which raised a foreign key violation
END
$$ language plpgsql;

create conflict trigger regress_conflict_trigger_insert on regress_conflictsub
   before foreign_key_violation
   when tag in ('INSERT')
   execute procedure foreign_key_insert_violation_before();

create conflict trigger regress_conflict_trigger_insert on regress_conflictsub
   after foreign_key_violation
   when tag in ('INSERT')
   execute procedure foreign_key_insert_violation_after();

The idea is that, for subscriptions that have conflict triggers defined, the apply will be wrapped in a PG_TRY()/PG_CATCH() block.  If it fails, the ErrorData will be copied in the ConflictTriggerContext, and then the transaction will be attempted again, but this time with any BEFORE and AFTER triggers applied.  The triggers could then return a special result indicating whether the transaction should be permanently skipped, applied, or whatever.  None of the data needs to be stored anywhere non-transient, as it just gets handed to the triggers.

I think the other patch is a subset of this functionality, as using this system to create triggers which query a table containing transactions to be skipped would be enough to get the functionality you've been discussing.  But this system could also do other things, like modify data.  Admittedly, this is akin to a statement level trigger and not a row level trigger, so a number of things you might want to do would be hard to do from this.  But perhaps the equivalent of row level triggers could also be written?

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Sun, 20 Jun 2021 22:16:21 -0700

> On Jun 20, 2021, at 10:11 PM, Amit Kapila <[email-masked]> wrote:
> 
> failing xids. 

I'm not sure we're talking about the same thing.  I'm saying that if the user is expected to clear each error manually, there could be many such errors for them to clear.  It may be true that the second error doesn't occur on the subscriber side until after the first is cleared, but that still leaves the user having to clear one after the next until arbitrarily many of them coming from the publisher side are cleared.

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Sun, 20 Jun 2021 22:25:48 -0700

> However, I see a different case where there could be

Right, I wasn't trying to capture everything, just enough to give the user a reasonable indication of what went wrong.  My patch was designed around the idea that the user would need to figure out how to fix the subscriber side prior to re-enabling the subscription.  As such, I wasn't bothered with trying to store everything, just enough to give the user a clue where to look.  I don't mind if you want to store more information, and maybe that needs to be stored somewhere else.  Do you believe pg_subscription_rel is a suitable location? 

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Mon, 21 Jun 2021 10:56:25 +0530

> the subscription is disabled automatically by the server.

True, but still there will be some information related to ERROR which
we wanted the user to see unless we ask them to refer to logs for
that.

> using another column to show the subscriber is disabled automatically

I agree that it is worth considering to have subenabled to have a
tri-state (enable, disabled, automatically-disabled) value instead of
just a boolean. But in this case, if the stats collector missed
updating the information, the user may have to manually update the
subscription and let the error happen again to see it.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 21 Jun 2021 11:19:58 +0530

On Mon, Jun 21, 2021 at 10:55 AM Mark Dilger
<[email-masked]> wrote:
> Right, I wasn't trying to capture everything, just enough to give the user a reasonable indication of what went wrong.  My patch was designed around the idea that the user would need to figure out how to fix the subscriber side prior to re-enabling the subscription.  As such, I wasn't bothered with trying to store everything, just enough to give the user a clue where to look.
>

Okay, but the clue will be pretty random because you might end up just
logging one out of several errors.

>  I don't mind if you want to store more information, and maybe that needs to be stored somewhere else.  Do you believe pg_subscription_rel is a suitable location?
>
It won't be sufficient to store information in either
pg_subscription_rel or pg_susbscription. I think if we want to store
the required information in a catalog then we need to define a new
catalog (pg_subscription_conflicts or something like that) with
information corresponding to each rel in subscription (srsubid oid
(Reference to subscription), srrelid oid (Reference to relation),
<columns for error_info>). OTOH, we can choose to send the error
information to stats collector which will then be available via stat
view and update system catalog to disable the subscription but there
will be a risk that we might send info of failed transaction to stats
collector but then fail to update system catalog to disable the
subscription.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 21 Jun 2021 16:17:48 +0530

> collector but then fail to update system catalog to disable the

I think we should store the input from the user (like disable_on_error
flag or xid to skip) in the system catalog pg_subscription and send
the error information (subscrtion_id, rel_id, xid of failed xact,
error_code, error_message, etc.) to the stats collector which can be
used to display such information via a stat view.

The disable_on_error flag handling could be that on error it sends the
required error info to stats collector and then updates the subenabled
in pg_subscription. In rare conditions, where we are able to send the
message but couldn't update the subenabled info in pg_subscription
either due to some error or server restart, the apply worker would
again try to apply the same change and would hit the same error again
which I think should be fine because it will ultimately succeed.

The skip xid handling will also be somewhat similar where on an error,
we will send the error information to stats collector which will be
displayed via stats view. Then the user is expected to ask for skip
xid (Alter Subscription ... SKIP <xid_value>) based on information
displayed via stat view. Now, the apply worker can skip changes from
such a transaction, and then during processing of commit record of the
skipped transaction, it should update xid to invalid value, so that
next time that shouldn't be used. I think it is important to update
xid to an invalid value as part of the skipped transaction because
otherwise, after the restart, we won't be able to decide whether we
still want to skip the xid stored for a subscription.

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Tue, 22 Jun 2021 10:57:40 +1000

Much of the discussion above seems to be related to where to store the
error information and how much information is needed to be useful.

As a summary, the 5 alternatives I have seen mentioned are:

#1. Store some simple message in the pg_subscription ("I wasn't trying
to capture everything, just enough to give the user a reasonable
indication of what went wrong" [Mark-1]). Storing the error message
was also seen as a convenience for writing TAP tests ("I originally
ran into the motivation to write this patch when frustrated that TAP
tests needed to parse the apply worker log file" [Mark-2}). It also
can sometimes provide a simple clue for the error (e.g. PK violation
for table TBL) but still the user will have to look elsewhere for
details to resolve the error. So while this implementation seems good
for simple scenarios, it appears to have been shot down because the
non-trivial scenarios either have insufficient or wrong information in
the error message. Some DETAILS could have been added to give more
information but that would maybe bloat the catalog ("I have not yet
included the DETAIL field because I didn't want to bloat the catalog."
[Mark-3])

#2. Similarly another idea was to use another existing catalog table
pg_subscription_rel. This could have the same problems ("It won't be
sufficient to store information in either pg_subscription_rel or
pg_susbscription." [Amit-1])

#3. There is another suggestion to use the Stats Collector to hold the
error message [Amit-2]. For me, this felt like blurring too much the
distinction between "stats tracking/metrics" and "logs". ERROR logs
must be flushed, whereas for stats (IIUC) there is no guarantee that
everything you need to see would be present. Indeed Amit wrote "But in
this case, if the stats collector missed updating the information, the
user may have to manually update the subscription and let the error
happen again to see it." [Amit-3]. Requesting the user to cause the
same error again just in case it was not captured a first time seems
too strange to me.

#4. The next idea was to have an entirely new catalog for holding the
subscription error information. I feel that storing/duplicating lots
of error information in another table seems like a bridge too far.
What about the risks of storing incorrect or sufficient information?
What is the advantage of duplicating errors over just referring to the
log files for ERROR details?

#5. Document to refer to the logs. All ERROR details are already in
the logs, and this seems to me the intuitive place to look for them.
Searching for specific errors becomes difficult programmatically (is
this really a problem other than complex TAP tests?). But here there
is no risk of missing or insufficient information captured in the log
files ("but still there will be some information related to ERROR
which we wanted the user to see unless we ask them to refer to logs
for that." [Amit-4}).

---

My preferred alternative is #5. ERRORs are logged in the log file, so
there is nothing really for this patch to do in this regard (except
documentation), and there is no risk of missing any information, no
ambiguity of having duplicated errors, and it is the intuitive place
the user would look.

So I felt current best combination is just this:
a) A tri-state indicating the state of the subscription: e.g.
something like "enabled" ('e')/ "disabled" ('d') / "auto-disabled"
('a') [Amit-5]
b) For "auto-disabled" the PG docs would be updated tell the user to
check the logs to resolve the problem before re-enabling the
subscription

//////////

IMO it is not made exactly clear to me what is the main goal of this
patch. Because of this, I feel that you can't really judge if this new
option is actually useful or not except only in hindsight. It seems
like whatever you implement can be made to look good or bad, just by
citing different test scenarios.

e.g.

* Is the goal mainly to help automated (TAP) testing? In that case,
then maybe you do want to store the error message somewhere other than
the log files. But still I wonder if results would be unpredictable
anyway - e.g if there are multiple tables all with errors then it
depends on the tablesync order of execution which error you see caused
the auto-disable, right? And if it is not predictable maybe it is less
useful.

* 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.

* 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? 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. 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.

* etc

//////////

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. 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=0" default, relaunch forever
"disable_on_error_threshold=1" disable upon first error encountered.
(This is how your patch behaves now I think.)
"disable_on_error_threshold=500" disable if the re-launch errors go
unattended and happen 500 times.

------
[Mark-1] https://www.postgresql.org/message-id/A539C848-670E-454F-B31C-82D3CBE9F5AC%40enterprisedb.com
[Mark-2] https://www.postgresql.org/message-id/DB35438F-9356-4841-89A0-412709EBD3AB%40enterprisedb.com
[Mark-3] https://www.postgresql.org/message-id/DE7E13B7-DC76-416A-A98F-3BC3F80E6BE9%40enterprisedb.com
[Amit-1] https://www.postgresql.org/message-id/CAA4eK1K_JFSFrAkr_fgp3VX6hTSmjK%3DwNs4Tw8rUWHGp0%2BBsaw%40mail.gmail.com
[Amit-2] https://www.postgresql.org/message-id/CAA4eK1%2BNoRbYSH1J08zi4OJ_EUMcjmxTwnmwVqZ6e_xzS0D6VA%40mail.gmail.com
[Amit-3] https://www.postgresql.org/message-id/CAA4eK1Kyx6U9yxC7OXoBD7pHC3bJ4LuNGd%3DOiABmiW6%2BqG%2BvEQ%40mail.gmail.com
[Amit-4] https://www.postgresql.org/message-id/CAA4eK1Kyx6U9yxC7OXoBD7pHC3bJ4LuNGd%3DOiABmiW6%2BqG%2BvEQ%40mail.gmail.com
[Amit-5] https://www.postgresql.org/message-id/CAA4eK1Kyx6U9yxC7OXoBD7pHC3bJ4LuNGd%3DOiABmiW6%2BqG%2BvEQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Mark Dilger <[email-masked]>
Date: Mon, 21 Jun 2021 19:29:38 -0700

> On Jun 21, 2021, at 5:57 PM, Peter Smith <[email-masked]> wrote:
> 

My original motivation came from writing TAP tests to check that the permissions systems would properly deny the apply worker when running under a non-superuser role.  The idea is that the user with the responsibility for managing subscriptions won't have enough privilege to read the logs.  Whatever information that user needs (if any) must be someplace else.

> this really a problem other than complex TAP tests?).

I believe there is a problem, because I remain skeptical that these errors will be both existent and rare.  Either you've configured your system correctly and you get zero of these, or you've misconfigured it and you get some non-zero number of them.  I don't see any reason to assume that number will be small.

The best way to deal with that is to be able to tell the system what to do with them, like "if the error has this error code and the error message matches this regular expression, then do this, else do that."  That's why I think allowing triggers to be created on subscriptions makes the most sense (though is probably the hardest system being proposed so far.)

> But here there

Not only is there a problem if the user doesn't have permission to view the logs, but also, if we automatically disable the subscription until the error is manually cleared, the logs might be rotated out of existence before the user takes any action.  In that case, the logs will be entirely missing, and not even the error message will remain.  At least with the patch I submitted, the error message will remain, though I take Amit's point that there are deficiencies in handling parallel tablesync workers, etc.

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Mon, 21 Jun 2021 19:35:38 -0700

> * Is the goal mainly to help automated (TAP) testing?

Absolutely, that was my original motivation.  But I don't think that is the primary reason the patch would be accepted.  There is a cost to having the logical replication workers attempt ad infinitum to apply a transaction that will never apply.

Also, if you are waiting for a subscription to catch up, it is far from obvious that you will wait forever.

> In that case,

But if you are writing a TAP test, you should be the one controlling whether that is the case.  I don't think it would be unpredictable from the point of view of the test author.

—
Mark Dilger

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 22 Jun 2021 11:42:04 +0900

> still want to skip the xid stored for a subscription.

Sounds reasonable.

The feature that sends the error information to the stats collector is
a common feature for both and itself is also useful. As discussed in
that skip transaction patch thread, it would also be good if we write
error information (relation, action, xid, etc) to the server log too.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Tue, 22 Jun 2021 08:14:09 +0530

> same error again just in case it was not captured a first time seems

I don't think it will often be the case that the stats collector will
miss updating the information. I am not feeling comfortable storing
error information in system catalogs. We have some other views which
capture somewhat similar conflict information
(pg_stat_database_conflicts) or failed transactions information. So, I
thought here we are extending the similar concept by storing some
additional information about errors.

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Mon, 21 Jun 2021 19:49:56 -0700

> should only be checking the apply worker like your original v1 patch

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. 

> 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
> each error in turn.

You are assuming disable_on_error=true.  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

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

> 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

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.

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Wed, 23 Jun 2021 08:23:31 +0530

> still want to skip the xid stored for a subscription.

One minor detail I missed in the above sketch for skipped transaction
feature was that actually we only need replication origin state from
the commit record of the skipped transaction and then I think we need
to start a transaction, update the xid value to invalid, set the
replication origin state and commit that transaction.

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 28 Jun 2021 13:47:11 +0900

> > On Jun 20, 2021, at 7:17 PM, Masahiko Sawada <[email-masked]> wrote:

I've submitted the patches on that thread[1]. There are three patches:
skipping the transaction on the subscriber side, reporting error
details in the errcontext, and reporting the error details to the
stats collector. Feedback is very welcome.

[1] https://www.postgresql.org/message-id/CAD21AoBU4jGEO6AXcykQ9y7tat0RrB5--8ZoJgfcj%2BLPs7nFZQ%40mail.gmail.com

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 2 Nov 2021 10:42:27 +0000

On Monday, June 28, 2021 1:47 PM Masahiko Sawada <[email-masked]> wrote:
> skipping the transaction on the subscriber side, reporting error details in the
> errcontext, and reporting the error details to the stats collector. Feedback is
> very welcome.
> 
> [1]
> https://www.postgresql.org/message-id/CAD21AoBU4jGEO6AXcykQ9y7tat0R
> rB5--8ZoJgfcj%2BLPs7nFZQ%40mail.gmail.com
Hi, thanks Sawada-san for keep updating the skip xid patch in the thread.

This thread has stopped since the patch submission.
I've rebased the 'disable_on_error' option
so that it can be applied on top of skip xid shared in [1].
I've written Mark Dilger as the original author in the commit message.

This patch is simply rebased to reactive this thread.
So there are still pending item to discuss for example,
how we should deal with multiple errors of several table sync workers.

I extracted only 'disable_on_error' option
because the skip xid and the latest error message fulfill the motivation
to make it easy to write TAP tests already I felt.

[1] - https://www.postgresql.org/message-id/CAD21AoDY-9_x819F_m1_wfCVXXFJrGiSmR2MfC9Nw4nW8Om0qA%40mail.gmail.com

Best Regards,
	Takamichi Osumi

---

From: vignesh C <[email-masked]>
Date: Mon, 8 Nov 2021 18:44:34 +0530

> to make it easy to write TAP tests already I felt.

Thanks for the updated patch. Please create a Commitfest entry for
this. It will help to have a look at CFBot results for the patch, also
if required rebase and post a patch on top of Head.

Regards,
Vignesh

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 10 Nov 2021 01:26:27 +0000

On Monday, November 8, 2021 10:15 PM vignesh C <[email-masked]> wrote:
> Thanks for the updated patch. Please create a Commitfest entry for this. It will
> help to have a look at CFBot results for the patch, also if required rebase and
> post a patch on top of Head.
As requested, created a new entry for this - [1]

FYI: the skip xid patch has been updated to v20 in [2]
but the v3 for disable_on_error is not affected by this update
and still applicable with no regression.

[1] - https://commitfest.postgresql.org/36/3407/
[2] - https://www.postgresql.org/message-id/CAD21AoAT42mhcqeB1jPfRL1%2BEUHbZk8MMY_fBgsyZvJeKNpG%2Bw%40mail.gmail.com

Best Regards,
	Takamichi Osumi

---

From: Greg Nancarrow <[email-masked]>
Date: Wed, 10 Nov 2021 15:22:50 +1100

> [2] - https://www.postgresql.org/message-id/CAD21AoAT42mhcqeB1jPfRL1%2BEUHbZk8MMY_fBgsyZvJeKNpG%2Bw%40mail.gmail.com

I had a look at this patch and have a couple of initial review
comments for some issues I spotted:

src/backend/commands/subscriptioncmds.c
(1) bad array entry assignment
The following code block added by the patch assigns
"values[Anum_pg_subscription_subdisableonerr - 1]" twice, resulting in
it being always set to true, rather than the specified option value:

+  if (IsSet(opts.specified_opts, SUBOPT_DISABLE_ON_ERR))
+  {
+    values[Anum_pg_subscription_subdisableonerr - 1]
+       = BoolGetDatum(opts.disableonerr);
+     values[Anum_pg_subscription_subdisableonerr - 1]
+       = true;
+  }

The 2nd line is meant to instead be
"replaces[Anum_pg_subscription_subdisableonerr - 1] = true".
(compare to handling for other similar options)

src/backend/replication/logical/worker.c
(2) unreachable code?
In the patch code there seems to be some instances of unreachable code
after re-throwing errors:

e.g.

+ /* If we caught an error above, disable the subscription */
+ if (disable_subscription)
+ {
+   ReThrowError(DisableSubscriptionOnError(cctx));
+   MemoryContextSwitchTo(ecxt);
+ }

+ else
+ {
+   PG_RE_THROW();
+   MemoryContextSwitchTo(ecxt);
+ }

+ if (disable_subscription)
+ {
+   ReThrowError(DisableSubscriptionOnError(cctx));
+   MemoryContextSwitchTo(ecxt);
+ }

I'm guessing it was intended to do the "MemoryContextSwitch(ecxt);"
before re-throwing (?), but it's not really clear, as in the 1st and
3rd cases, the DisableSubscriptionOnError() calls anyway immediately
switch the memory context to cctx.

Regards,
Greg Nancarrow
Fujitsu Australia

---

From: Greg Nancarrow <[email-masked]>
Date: Wed, 10 Nov 2021 15:31:19 +1100

> comments for some issues I spotted:

Incidentally, I found that the v3 patch only applies after the skip xid v20
patch [1] has been applied.

[2] -
https://www.postgresql.org/message-id/CAD21AoAT42mhcqeB1jPfRL1%2BEUHbZk8MMY_fBgsyZvJeKNpG%2Bw%40mail.gmail.com

Regards,
Greg Nancarrow
Fujitsu Australia

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 11 Nov 2021 09:20:44 +0000

On Wednesday, November 10, 2021 1:23 PM Greg Nancarrow <[email-masked]> wrote:
> > > this. It will help to have a look at CFBot results for the patch,
> > > also if required rebase and post a patch on top of Head.
> > FYI: the skip xid patch has been updated to v20 in [2] but the v3 for
> > disable_on_error is not affected by this update and still applicable
> > with no regression.
> >
> https://www.postgresql.org/message-id/CAD21AoAT42mhcqeB1jPfRL1%2B
> EUHbZ
> > k8MMY_fBgsyZvJeKNpG%2Bw%40mail.gmail.com
> 
> I had a look at this patch and have a couple of initial review comments for some
> issues I spotted:
Thank you for checking it.

> "values[Anum_pg_subscription_subdisableonerr - 1]" twice, resulting in it
> being always set to true, rather than the specified option value:
> 
> +  if (IsSet(opts.specified_opts, SUBOPT_DISABLE_ON_ERR))  {
Oops, fixed.
 
> In the patch code there seems to be some instances of unreachable code after
> re-throwing errors:
> 
> e.g.
> 
> + /* If we caught an error above, disable the subscription */ if
> + (disable_subscription) {
> before re-throwing (?), but it's not really clear, as in the 1st and 3rd cases, the
> DisableSubscriptionOnError() calls anyway immediately switch the memory
> context to cctx.
You are right I think.
Fixed based on an idea below.

After an error happens, for some additional work
(e.g. to report the stats of table sync/apply worker
by pgstat_report_subworker_error() or
to update the catalog by DisableSubscriptionOnError())
restore the memory context that is used before the error (cctx)
and save the old memory context of error (ecxt). Then,
do the additional work and switch the memory context to the ecxt
just before the rethrow. As you described, 
in contrast to PG_RE_THROW, DisableSubscriptionOnError() changes
the memory context immediatedly at the top of it,
so for this case, I don't call the MemoryContextSwitchTo().

Another important thing as my modification
is a case when LogicalRepApplyLoop failed and
apply_error_callback_arg.command == 0. In the original
patch of skip xid, it just calls PG_RE_THROW()
but my previous v3 codes missed this macro in this case.
Therefore, I've fixed this part as well.

C codes are checked by pgindent.

Note that this depends on the v20 skip xide patch in [1]

[1] - https://www.postgresql.org/message-id/CAD21AoAT42mhcqeB1jPfRL1%2BEUHbZk8MMY_fBgsyZvJeKNpG%2Bw%40mail.gmail.com

Best Regards,
	Takamichi Osumi

---

From: vignesh C <[email-masked]>
Date: Fri, 12 Nov 2021 09:38:35 +0530

> Note that this depends on the v20 skip xide patch in [1]

Thanks for the updated patch, Few comments:
1)  tab completion should be added for disable_on_error:
/* Complete "CREATE SUBSCRIPTION <name> ...  WITH ( <opt>" */
else if (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
COMPLETE_WITH("binary", "connect", "copy_data", "create_slot",
  "enabled", "slot_name", "streaming",
  "synchronous_commit", "two_phase");

2) disable_on_error is supported by alter subscription, the same
should be documented:
@ -871,11 +886,19 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
                        {
                                supported_opts = (SUBOPT_SLOT_NAME |

SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
-
SUBOPT_STREAMING);
+
SUBOPT_STREAMING | SUBOPT_DISABLE_ON_ERR);

                                parse_subscription_options(pstate,
stmt->options,

            supported_opts, &opts);

+                               if (IsSet(opts.specified_opts,
SUBOPT_DISABLE_ON_ERR))
+                               {
+
values[Anum_pg_subscription_subdisableonerr - 1]
+                                               =
BoolGetDatum(opts.disableonerr);
+
replaces[Anum_pg_subscription_subdisableonerr - 1]
+                                               = true;
+                               }
+

3) Describe subscriptions (dRs+) should include displaying of disableonerr:
\dRs+ sub1
                                                      List of subscriptions
 Name |  Owner  | Enabled | Publication | Binary | Streaming | Two
phase commit | Synchronous commit |         Conninfo
------+---------+---------+-------------+--------+-----------+------------------+--------------------+---------------------------
 sub1 | vignesh | t       | {pub1}      | f      | f         | d
         | off                | dbname=postgres port=5432
(1 row)

4) I felt transicent should be transient, might be a typo:
+          Specifies whether the subscription should be automatically disabled
+          if replicating data from the publisher triggers non-transicent errors
+          such as referential integrity or permissions errors. The default is
+          <literal>false</literal>.

5) The commented use PostgresNode and use TestLib can be removed:
+# Test of logical replication subscription self-disabling feature
+use strict;
+use warnings;
+# use PostgresNode;
+# use TestLib;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More tests => 10;

Regards,
Vignesh

---

From: Greg Nancarrow <[email-masked]>
Date: Fri, 12 Nov 2021 15:48:59 +1100

> Note that this depends on the v20 skip xide patch in [1]

Some comments on the v4 patch:

(1) Patch subject
I think the patch subject should say "disable" instead of "disabling":
   Optionally disable subscriptions on error

doc/src/sgml/ref/create_subscription.sgml
(2) spelling mistake
+          if replicating data from the publisher triggers non-transicent errors

non-transicent -> non-transient

(I notice Vignesh also pointed this out)

src/backend/replication/logical/worker.c
(3) calling geterrcode()
The new IsSubscriptionDisablingError() function calls geterrcode().
According to the function comment for geterrcode(), it is only
intended for use in error callbacks.
Instead of calling geterrcode(), couldn't the ErrorData from PG_CATCH
block be passed to IsSubscriptionDisablingError() instead (from which
it can get the sqlerrcode)?

(4) DisableSubscriptionOnError
DisableSubscriptionOnError() is again calling MemoryContextSwitch()
and CopyErrorData().
I think the ErrorData from the PG_CATCH block could simply be passed
to DisableSubscriptionOnError() instead of the memory-context, and the
existing MemoryContextSwitch() and CopyErrorData() calls could be
removed from it.

AFAICS, applying (3) and (4) above would make the code a lot cleaner.

Regards,
Greg Nancarrow
Fujitsu Australia

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 16 Nov 2021 07:53:23 +0000

On Friday, November 12, 2021 1:09 PM vignesh C <[email-masked]> wrote:
> /* Complete "CREATE SUBSCRIPTION <name> ...  WITH ( <opt>" */ else if
> (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
Fixed.

> 2) disable_on_error is supported by alter subscription, the same should be
> documented:
Fixed the documentation. Also, add one test for alter subscription.

> ------+---------+---------+-------------+--------+-----------+----------
> --------+--------------------+---------------------------
Fixed.

> +          <literal>false</literal>.
Fixed.

> +# Test of logical replication subscription self-disabling feature use
> +strict; use warnings; # use PostgresNode; # use TestLib; use
> +PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More
> +tests => 10;
Removed.

Also, my colleague Greg provided an offlist patch to me and
I've incorporated his suggested modifications into this version.
So, I noted his name as a coauthor.

C codes are checked by pgindent again.

This v5 depends on v23 skip xid in [1].

[1] - https://www.postgresql.org/message-id/CAD21AoA5jupM6O%3DpYsyfaxQ1aMX-en8%3DQNgpW6KfXsg7_CS0CQ%40mail.gmail.com

Best Regards,
	Takamichi Osumi

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 16 Nov 2021 07:59:36 +0000

Thank you for checking the patch !

Fixed.

> +          if replicating data from the publisher triggers
> + non-transicent errors
> 
Fixed.

> According to the function comment for geterrcode(), it is only intended for use
> in error callbacks.
> Instead of calling geterrcode(), couldn't the ErrorData from PG_CATCH block be
> passed to IsSubscriptionDisablingError() instead (from which it can get the
> sqlerrcode)?
> 
> DisableSubscriptionOnError() is again calling MemoryContextSwitch() and
> CopyErrorData().
> I think the ErrorData from the PG_CATCH block could simply be passed to
> DisableSubscriptionOnError() instead of the memory-context, and the existing
> MemoryContextSwitch() and CopyErrorData() calls could be removed from it.
> 
Fixed.

The updated patch is shared in [1].

[1] - https://www.postgresql.org/message-id/TYCPR01MB8373771371B31E1E6CC74B0AED999%40TYCPR01MB8373.jpnprd01.prod.outlook.com

Best Regards,
	Takamichi Osumi

---

From: Greg Nancarrow <[email-masked]>
Date: Thu, 18 Nov 2021 16:07:34 +1100

> This v5 depends on v23 skip xid in [1].

A minor comment:

doc/src/sgml/ref/alter_subscription.sgml
(1) disable_on_err?

+      <literal>disable_on_err</literal>.

This doc update names the new parameter as "disable_on_err" instead of
"disable_on_error".
Also "disable_on_err" appears in a couple of the test case comments.

Regards,
Greg Nancarrow
Fujitsu Australia

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 18 Nov 2021 07:22:15 +0000

> A minor comment:
Thanks for your comments !
 
> Also "disable_on_err" appears in a couple of the test case comments.
Fixed all 3 places.

At the same time, I changed one function name
from IsSubscriptionDisablingError() to IsTransientError()
so that it can express what it checks correctly.
Of course, the return value of true or false
becomes reverse by this name change, but
This would make the function more general.
Also, its comments were fixed.

This version also depends on the v23 of skip xid [1]

Best Regards,
	Takamichi Osumi

---

From: vignesh C <[email-masked]>
Date: Mon, 22 Nov 2021 12:22:56 +0530

> This version also depends on the v23 of skip xid [1]

Few comments:
1) Changes to handle pg_dump are missing. It should be done in
dumpSubscription and getSubscriptions

2) "And" is missing
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -201,8 +201,8 @@ ALTER SUBSCRIPTION <replaceable
class="parameter">name</replaceable> RENAME TO <
       information.  The parameters that can be altered
       are <literal>slot_name</literal>,
       <literal>synchronous_commit</literal>,
-      <literal>binary</literal>, and
-      <literal>streaming</literal>.
+      <literal>binary</literal>,<literal>streaming</literal>
+      <literal>disable_on_error</literal>.
Should be:
-      <literal>binary</literal>, and
-      <literal>streaming</literal>.
+      <literal>binary</literal>,<literal>streaming</literal>, and
+      <literal>disable_on_error</literal>.

3) Should we change this :
+          Specifies whether the subscription should be automatically disabled
+          if replicating data from the publisher triggers non-transient errors
+          such as referential integrity or permissions errors. The default is
+          <literal>false</literal>.
to:
+          Specifies whether the subscription should be automatically disabled
+          while replicating data from the publisher triggers
non-transient errors
+          such as referential integrity, permissions errors, etc. The
default is
+          <literal>false</literal>.

Regards,
Vignesh

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 26 Nov 2021 14:36:35 +0000

> Few comments:
Thank you so much for your review !

Fixed.

Fixed.

> +          <literal>false</literal>.
I preferred the previous description. The option
"disable_on_error" works with even one error.
If we use "while", the nuance would be like
we keep disabling a subscription more than once.
This situation happens only when user makes
the subscription enable without resolving the non-transient error,
which sounds a bit unnatural. So, I wanna keep the previous description.
If you are not satisfied with this, kindly let me know.

This v7 uses v26 of skip xid patch [1]

[1] - https://www.postgresql.org/message-id/CAD21AoDNe_O%2BCPucd_jQPu3gGGaCLNP%2BJ_kSPNecTdAM8HFPww%40mail.gmail.com

Best Regards,
	Takamichi Osumi

---

From: vignesh C <[email-masked]>
Date: Mon, 29 Nov 2021 11:07:55 +0530

Thanks for the updated patch, Few comments:
1) Since this function is used only from 027_disable_on_error and not
used by others, this can be moved to 027_disable_on_error:
+sub wait_for_subscriptions
+{
+       my ($self, $dbname, @subscriptions) = @_;
+
+       # Unique-ify the subscriptions passed by the caller
+       my %unique = map { $_ => 1 } @subscriptions;
+       my @unique = sort keys %unique;
+       my $unique_count = scalar(@unique);
+
+       # Construct a SQL list from the unique subscription names
+       my @escaped = map { s/'/''/g; s/\\/\\\\/g; $_ } @unique;
+       my $sublist = join(', ', map { "'$_'" } @escaped);
+
+       my $polling_sql = qq(
+               SELECT COUNT(1) = $unique_count FROM
+                       (SELECT s.oid
+                               FROM pg_catalog.pg_subscription s
+                               LEFT JOIN pg_catalog.pg_subscription_rel sr
+                               ON sr.srsubid = s.oid
+                               WHERE (sr IS NULL OR sr.srsubstate IN
('s', 'r'))
+                                 AND s.subname IN ($sublist)
+                                 AND s.subenabled IS TRUE
+                        UNION
+                        SELECT s.oid
+                               FROM pg_catalog.pg_subscription s
+                               WHERE s.subname IN ($sublist)
+                                 AND s.subenabled IS FALSE
+                       ) AS synced_or_disabled
+               );
+       return $self->poll_query_until($dbname, $polling_sql);
+}

2) The empty line after comment is not required, it can be removed
+# Create non-unique data in both schemas on the publisher.
+#
+for $schema (@schemas)
+{

3) Similarly it can be changed across the file
+# Wait for the initial subscription synchronizations to finish or fail.
+#
+$node_subscriber->wait_for_subscriptions('postgres', @schemas)
+       or die "Timed out while waiting for subscriber to synchronize data";

+# Enter unique data for both schemas on the publisher.  This should succeed on
+# the publisher node, and not cause any additional problems on the subscriber
+# side either, though disabled subscription "s1" should not replicate anything.
+#
+for $schema (@schemas)

4) Since subid is used only at one place, no need of subid variable,
you could replace subid with subform->oid in LockSharedObject
+       Datum           values[Natts_pg_subscription];
+       HeapTuple       tup;
+       Oid                     subid;
+       Form_pg_subscription subform;

+       subid = subform->oid;
+       LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock);

5) "permissions errors" should be "permission errors"
+         </para>

Regards,
Vignesh

---

From: Greg Nancarrow <[email-masked]>
Date: Tue, 30 Nov 2021 15:09:59 +1100

> This v7 uses v26 of skip xid patch [1]

This patch no longer applies on the latest source.
Also, the patch is missing an update to doc/src/sgml/catalogs.sgml,
for the new "subdisableonerr" column of pg_subscription.

Regards,
Greg Nancarrow
Fujitsu Australia

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 30 Nov 2021 12:04:14 +0000

On Tuesday, November 30, 2021 1:10 PM Greg Nancarrow <[email-masked]> wrote:
> Also, the patch is missing an update to doc/src/sgml/catalogs.sgml, for the
> new "subdisableonerr" column of pg_subscription.
Thanks for your review !

Fixed the documentation accordingly. Further,
this comment invoked some more refactoring of codes
since I wrote some internal codes related to
'disable_on_error' in an inconsistent order.
I fixed this by keeping patch's codes
after that of 'two_phase' subscription option as much as possible.

I also conducted both pgindent and pgperltidy.

Now, I'll share the v8 that uses PG
whose commit id is after 8d74fc9 (pg_stat_subscription_workers).

Best Regards,
	Takamichi Osumi

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 30 Nov 2021 12:13:29 +0000

On Monday, November 29, 2021 2:38 PM vignesh C <[email-masked]>
> Thanks for the updated patch, Few comments:
Thank you for your review !

> 1) Since this function is used only from 027_disable_on_error and not used by
> others, this can be moved to 027_disable_on_error:
> +       return $self->poll_query_until($dbname, $polling_sql); }
Fixed.

Fixed.

> +       or die "Timed out while waiting for subscriber to synchronize
> +data";
> 
> +# Enter unique data for both schemas on the publisher.  This should
> +succeed on # the publisher node, and not cause any additional problems
> +on the subscriber # side either, though disabled subscription "s1" should not
> replicate anything.
> +#
> +for $schema (@schemas)
Fixed.
 
> 4) Since subid is used only at one place, no need of subid variable, you could
> replace subid with subform->oid in LockSharedObject
> +       LockSharedObject(SubscriptionRelationId, subid, 0,
> + AccessExclusiveLock);
Fixed.

Fixed.

The new patch v8 is shared in [1].

[1] - https://www.postgresql.org/message-id/TYCPR01MB83735AA021E0F614A3AB3221ED679%40TYCPR01MB8373.jpnprd01.prod.outlook.com

Best Regards,
	Takamichi Osumi

---

From: vignesh C <[email-masked]>
Date: Wed, 1 Dec 2021 11:31:32 +0530

> whose commit id is after 8d74fc9 (pg_stat_subscription_workers).

Thanks for the updated patch, few small comments:
1) This should be changed:
+       <structfield>subdisableonerr</structfield> <type>bool</type>
+      </para>
+      <para>
+       If true, the subscription will be disabled when subscription
+       worker detects an error
+      </para></entry>
+     </row>

to:
+       <structfield>subdisableonerr</structfield> <type>bool</type>
+      </para>
+      <para>
+       If true, the subscription will be disabled when subscription
+       worker detects non-transient errors
+      </para></entry>
+     </row>

2) "Disable On Err" can be changed to "Disable On Error"
+                                                         ",
subtwophasestate AS \"%s\"\n"
+                                                         ",
subdisableonerr AS \"%s\"\n",
+
gettext_noop("Two phase commit"),
+
gettext_noop("Disable On Err"));

3) Can add a line in the commit message saying "Bump catalog version."
as the patch involves changing the catalog.

4) This prototype is not required, since the function is called after
the function definition:
 static inline void set_apply_error_context_xact(TransactionId xid,
TimestampTz ts);
 static inline void reset_apply_error_context_info(void);
+static bool IsTransientError(ErrorData *edata);

5) we could use the new style here:
+       ereport(LOG,
+                       (errmsg("logical replication subscription
\"%s\" will be disabled due to error: %s",
+                                       MySubscription->name, edata->message)));

change it to:
+       ereport(LOG,
+                       errmsg("logical replication subscription
\"%s\" will be disabled due to error: %s",
+                                       MySubscription->name, edata->message));

Similarly it can be changed in the other ereports added.

Regards,
Vignesh

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 1 Dec 2021 12:25:35 +0000

On Wednesday, December 1, 2021 3:02 PM vignesh C <[email-masked]> wrote:
> > Fixed the documentation accordingly. Further, this comment invoked
> > some more refactoring of codes since I wrote some internal codes
> > related to 'disable_on_error' in an inconsistent order.
I appreciate your check.

> +       worker detects non-transient errors
Fixed. Actually, there's no clear definition what "non-transient" means
in the documentation. So, I added some words to your suggestion,
which would give clearer understanding to users.

Fixed.

> as the patch involves changing the catalog.
Hmm, let me postpone this fix till the final version.
The catalog version gets easily updated by other patch commits
and including it in the middle of development can become
cause of conflicts of my patch when applied to the PG,
which is possible to make other reviewers stop reviewing.

> 4) This prototype is not required, since the function is called after the function
> definition:
> TimestampTz ts);  static inline void reset_apply_error_context_info(void);
Fixed.

> +                                       MySubscription->name,
> + edata->message)));
> +                                       MySubscription->name,
> + edata->message));
Removed the unnecessary parentheses.

Best Regards,
	Takamichi Osumi

---

From: Amit Kapila <[email-masked]>
Date: Wed, 1 Dec 2021 18:46:24 +0530

> which is possible to make other reviewers stop reviewing.

Vignesh seems to be suggesting just changing the commit message, not
the actual code. This is sort of a reminder to the committer to change
the catversion before pushing the patch. So that shouldn't cause any
conflicts while applying your patch.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 2 Dec 2021 01:05:16 +0000

On Wednesday, December 1, 2021 10:16 PM Amit Kapila <[email-masked]> wrote:
> > The catalog version gets easily updated by other patch commits and
> > including it in the middle of development can become cause of
> > conflicts of my patch when applied to the PG, which is possible to
> > make other reviewers stop reviewing.
> >
> 
> Vignesh seems to be suggesting just changing the commit message, not the
> actual code. This is sort of a reminder to the committer to change the catversion
> before pushing the patch. So that shouldn't cause any conflicts while applying
> your patch.
Ah, sorry for my misunderstanding.
Updated the patch to include the notification.

Best Regards,
	Takamichi Osumi

---

From: Greg Nancarrow <[email-masked]>
Date: Thu, 2 Dec 2021 13:42:47 +1100

> Updated the patch to include the notification.

For the catalogs.sgml update, I was thinking that the following
wording might sound a bit better:

+       If true, the subscription will be disabled when a subscription
+       worker detects non-transient errors (e.g. duplication error)
+       that require user intervention to resolve.

What do you think?

Regards,
Greg Nancarrow
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Thu, 2 Dec 2021 10:18:59 +0530

> Updated the patch to include the notification.

The patch disables the subscription for non-transient errors. I am not
sure if we can easily make the call to decide whether any particular
error is transient or not. For example, DISK_FULL or OUT_OF_MEMORY
might not rectify itself. Why not just allow to disable the
subscription on any error? And then let the user check the error
either in view or logs and decide whether it would like to enable the
subscription or do something before it (like making space in disk, or
fixing the network).

The other problem I see with this transient error stuff is maintaining
the list of error codes that we think are transient. I think we need a
discussion for each of the error_codes we are listing now and whatever
new error_code we add in the future which doesn't seem like a good
idea.

I think the code to deal with apply worker errors and then disable the
subscription has some flaws. Say, while disabling the subscription if
it leads to another error then I think the original error won't be
reported.  Can't we simply emit the error via EmitErrorReport and then
do AbortOutOfAnyTransaction, FlushErrorState, and any other memory
context clean up if required and then disable the subscription after
coming out of catch?

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 2 Dec 2021 07:40:44 +0000

On Thursday, December 2, 2021 1:49 PM Amit Kapila <[email-masked]> wrote:
> The patch disables the subscription for non-transient errors. I am not sure if we
> can easily make the call to decide whether any particular error is transient or
> not. For example, DISK_FULL or OUT_OF_MEMORY might not rectify itself.
> Why not just allow to disable the subscription on any error? And then let the
> user check the error either in view or logs and decide whether it would like to
> enable the subscription or do something before it (like making space in disk, or
> fixing the network).
Agreed. I'll treat any errors as the trigger of the feature
in the next version.

> The other problem I see with this transient error stuff is maintaining the list of
> error codes that we think are transient. I think we need a discussion for each of
> the error_codes we are listing now and whatever new error_code we add in the
> future which doesn't seem like a good idea.
This is also true. The maintenance cost of my current implementation
didn't sound cheap.

> subscription has some flaws. Say, while disabling the subscription if it leads to
> another error then I think the original error won't be reported.  Can't we simply
> emit the error via EmitErrorReport and then do AbortOutOfAnyTransaction,
> FlushErrorState, and any other memory context clean up if required and then
> disable the subscription after coming out of catch?
You are right. I'll fix related parts accordingly.

Best Regards,
	Takamichi Osumi

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 3 Dec 2021 13:20:35 +0000

Thursday, December 2, 2021 4:41 PM I wrote:
> might not rectify itself.
> > Why not just allow to disable the subscription on any error? And then
> > let the user check the error either in view or logs and decide whether
> > it would like to enable the subscription or do something before it
> > (like making space in disk, or fixing the network).
> Agreed. I'll treat any errors as the trigger of the feature in the next version.
> 
> > new error_code we add in the future which doesn't seem like a good idea.
> This is also true. The maintenance cost of my current implementation didn't
> sound cheap.
> 
> > context clean up if required and then disable the subscription after coming
> out of catch?
Hi, I've made a new patch v11 that incorporated suggestions described above.

There are several notes to share regarding v11 modifications.

1. Modified the commit message a bit.

2. DisableSubscriptionOnError() doesn't return ErrData anymore,
since now to emit error message is done in the error recovery area
and the function purpose has become purely to run a transaction to disable
the subscription.

3. In DisableSubscriptionOnError(), v10 rethrew the error if the disable_on_error
flag became false in the interim, but v11 just closes the transaction and
finishes the function.

4. If table sync worker detects an error during synchronization
and needs to disable the subscription, the worker disables it and just exit by proc_exit.
The processing after disabling the subscription didn't look necessary to me
for disabled subscription.

5. Only when we succeed in the table synchronization, it's necessary to
allocate slot name in long-lived context, after the table synchronization in
ApplyWorkerMain(). Otherwise, we'll see junk value of syncslotname
because it is the return value of LogicalRepSyncTableStart().

6. There are 3 places for error recovery in ApplyWorkerMain().
All of those might look similar but I didn't make an united function for them.
Those are slightly different from each other and I felt
readability is reduced by grouping them into one type of function call.

7. In v11, I covered the case that apply worker failed with
apply_error_callback_arg.command == 0, as one path to disable the subscription
in order to cover all errors.

8. I changed one flag name from 'disable_subscription' to 'did_error'
in ApplyWorkerMain().

9. All chages in this version are C codes and checked by pgindent.

Best Regards,
	Takamichi Osumi

---

From: Greg Nancarrow <[email-masked]>
Date: Mon, 6 Dec 2021 15:16:18 +1100

> 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

---

From: Mark Dilger <[email-masked]>
Date: Sun, 5 Dec 2021 20:37:44 -0800

> On Dec 1, 2021, at 8:48 PM, Amit Kapila <[email-masked]> wrote:
> 

The original idea of the patch, back when I first wrote and proposed it, was to remove the *absurdity* of retrying a transaction which, in the absence of human intervention, was guaranteed to simply fail again ad infinitum.  Retrying in the face of resource errors is not *absurd* even though it might fail again ad infinitum.  The reason is that there is at least a chance that the situation will clear up without human intervention.

> new error_code we add in the future which doesn't seem like a good

A reasonable rule might be:  "the subscription will be disabled if the server can determine that retries cannot possibly succeed without human intervention."  We shouldn't need to categorize all error codes perfectly, as long as we're conservative.  What I propose is similar to how we determine whether to mark a function leakproof; we don't have to mark all leakproof functions as such, we just can't mark one as such if it is not.

If we're going to debate the error codes, I think we would start with an empty list, and add to the list on sufficient analysis.

—
Mark Dilger

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 6 Dec 2021 06:56:16 +0000

On Monday, December 6, 2021 1:38 PM Mark Dilger <[email-masked]> wrote:
> The original idea of the patch, back when I first wrote and proposed it, was to
> remove the *absurdity* of retrying a transaction which, in the absence of
> human intervention, was guaranteed to simply fail again ad infinitum.
> Retrying in the face of resource errors is not *absurd* even though it might fail
> again ad infinitum.  The reason is that there is at least a chance that the
> situation will clear up without human intervention.
In my humble opinion, I felt the original purpose of the patch was to partially remedy
the situation that during the failure of apply, the apply process keeps going
into the infinite error loop.

I'd say that in this sense, if we include such resource errors, we fail to achieve
the purpose in some cases, because of some left possibilities of infinite loop.
Disabling the subscription with even one any error excludes this irregular possibility,
since there's no room to continue the infinite loop.

Best Regards,
	Takamichi Osumi

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 6 Dec 2021 10:52:32 +0000

> Some review comments for the v11 patch:
Thank you for your reviews !
 
Fixed.

> /* Reset the ErrorContext */
Agreed. Fixed.
 
Fixed.
 
> WorkerErrorRecovery() was called.
Adopted error_recovery_done
and changed its places accordingly.
 
>  "logical replication subscription \"%s\" will be disabled due to an error"
Added.

I've attached the new version v12.

Best Regards,
	Takamichi Osumi

---

From: Amit Kapila <[email-masked]>
Date: Mon, 6 Dec 2021 16:29:06 +0530

> If we're going to debate the error codes, I think we would start with an empty list, and add to the list on sufficient analysis.

Yeah, an empty list is a sort of what I thought was a good start
point. I feel we should learn from real-world use cases to see if
people really want to continue retrying even after using this option.

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Mon, 6 Dec 2021 08:06:02 -0800

> On Dec 5, 2021, at 10:56 PM, [email-masked] wrote:
> 

I agree.

> since there's no room to continue the infinite loop.

I don't think there is any right answer here.  It's a question of policy preferences.

My concern about disabling a subscription in response to *any* error is that people may find the feature does more harm than good.  Disabling the subscription in response to an occasional deadlock against other database users, or occasional resource pressure, might annoy people and lead to the feature simply not being used.

I am happy to defer to your policy preference.  Thanks for your work on the patch!

—
Mark Dilger

---

From: Greg Nancarrow <[email-masked]>
Date: Tue, 7 Dec 2021 11:22:08 +1100

> My concern about disabling a subscription in response to *any* error is that people may find the feature does more harm than good.  Disabling the subscription in response to an occasional deadlock…
I can understand this point of view.
It kind of suggests to me the possibility of something like a
configurable timeout (e.g. disable the subscription if the same error
has occurred for more than X minutes) or, similarly, perhaps if some
threshold has been reached (e.g. same error has occurred more than X
times), but I think that this was previously suggested by Peter Smith
and the idea wasn't looked upon all that favorably?

Regards,
Greg Nancarrow
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Wed, 8 Dec 2021 18:40:59 +0530

> and the idea wasn't looked upon all that favorably?

I think if we are really worried about transient errors then probably
the idea "disable only if the same error has occurred more than X
times" seems preferable as compared to taking a decision on which
error_codes fall in the transient error category.

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Wed, 8 Dec 2021 07:52:40 -0800

> On Dec 8, 2021, at 5:10 AM, Amit Kapila <[email-masked]> wrote:
> 

No need.  We can revisit this design decision in a later release cycle if the current patch's design proves problematic in the field.

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Thu, 9 Dec 2021 09:39:53 +0530

> No need.  We can revisit this design decision in a later release cycle if the current patch's design proves problematic in the field.

So, do you agree that we can disable the subscription on any error if
this parameter is set?

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Thu, 9 Dec 2021 08:44:50 -0800

> On Dec 8, 2021, at 8:09 PM, Amit Kapila <[email-masked]> wrote:
> 

Yes, I think that is fine.  We can commit it that way, and revisit the issue for v16 if it becomes a problem in practice.

—
Mark Dilger

---

From: vignesh C <[email-masked]>
Date: Mon, 13 Dec 2021 15:27:17 +0530

> I've attached the new version v12.

Thanks for the updated patch, few comments:
1) This is not required as it is not used in the caller.
+++ b/src/backend/replication/logical/launcher.c
@@ -132,6 +132,7 @@ get_subscription_list(void)
                sub->dbid = subform->subdbid;
                sub->owner = subform->subowner;
                sub->enabled = subform->subenabled;
+               sub->disableonerr = subform->subdisableonerr;
                sub->name = pstrdup(NameStr(subform->subname));
                /* We don't fill fields we are not interested in. */

2) Should this be changed:
+       <structfield>subdisableonerr</structfield> <type>bool</type>
+      </para>
+      <para>
+       If true, the subscription will be disabled when subscription
+       worker detects any errors
+      </para></entry>
+     </row>
To:
+       <structfield>subdisableonerr</structfield> <type>bool</type>
+      </para>
+      <para>
+       If true, the subscription will be disabled when subscription's
+       worker detects any errors
+      </para></entry>
+     </row>

3) The last line can be slightly adjusted to keep within 80 chars:
+          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>.

4) Similarly this too can be handled:
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1259,7 +1259,7 @@ REVOKE ALL ON pg_replication_origin_status FROM public;
 -- All columns of pg_subscription except subconninfo are publicly readable.
 REVOKE ALL ON pg_subscription FROM public;
 GRANT SELECT (oid, subdbid, subname, subowner, subenabled, subbinary,
-              substream, subtwophasestate, subslotname,
subsynccommit, subpublications)
+              substream, subtwophasestate, subdisableonerr,
subslotname, subsynccommit, subpublications)
     ON pg_subscription TO public;

5) Since disabling subscription code is common in and else, can we
move it below:
+                       if (MySubscription->disableonerr)
+                       {
+                               WorkerErrorRecovery();
+                               error_recovery_done = true;
+                       }
+                       else
+                       {
+                               /*
+                                * Some work in error recovery work is
done. Switch to the old
+                                * memory context and rethrow.
+                                */
+                               MemoryContextSwitchTo(ecxt);
+                               PG_RE_THROW();
+                       }
+               }
+               else
+               {
+                       /*
+                        * Don't miss any error, even when it's not
reported to stats
+                        * collector.
+                        */
+                       if (MySubscription->disableonerr)
+                       {
+                               WorkerErrorRecovery();
+                               error_recovery_done = true;
+                       }
+                       else
+                               /* Simply rethrow because of no recovery work */
+                               PG_RE_THROW();
+               }

6) Can we move LockSharedObject below the if condition.
+       subform = (Form_pg_subscription) GETSTRUCT(tup);
+       LockSharedObject(SubscriptionRelationId, subform->oid, 0,
AccessExclusiveLock);
+
+       /*
+        * 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;
+       }
+

Regards,
Vignesh

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 14 Dec 2021 05:34:53 +0000

> > I've attached the new version v12.
I appreciate your review.

>                 /* We don't fill fields we are not interested in. */
Okay.
The comment of the get_subscription_list() mentions that
we collect and fill only fields related to worker start/stop.
Then, I didn't need it. Fixed.

> +       worker detects any errors
I felt either is fine. So fixed.

Fixed.

>  REVOKE ALL ON pg_subscription FROM public;  GRANT SELECT (oid,
> subdbid, subname, subowner, subenabled, subbinary,
I split the line into two to make each line less than 80 chars.

> 5) Since disabling subscription code is common in and else, can we move it
> below:
I moved the common code below those condition branches.

Fixed.

Besides all of those changes, I've removed the obsolete
comment of DisableSubscriptionOnError in v12.

Best Regards,
	Takamichi Osumi

---

From: Greg Nancarrow <[email-masked]>
Date: Thu, 16 Dec 2021 16:32:08 +1100

> comment of DisableSubscriptionOnError in v12.

I have a few minor comments, otherwise the patch LGTM at this point:

doc/src/sgml/catalogs.sgml
(1)
Current comment says:

However, in create_subscription.sgml, it says "disabled if any errors
are detected by subscription workers ..."

For consistency, I think it should be:

+       If true, the subscription will be disabled when subscription
+       workers detect any errors

src/bin/psql/describe.c
(2)
I think that:

+  gettext_noop("Disable On Error"));

should be:

+  gettext_noop("Disable on error"));

for consistency with the uppercase/lowercase usage on other similar entries?
(e.g. "Two phase commit")

src/include/catalog/pg_subscription.h
(3)

+  bool subdisableonerr; /* True if apply errors should disable the
+                                       * subscription upon error */

The comment should just say "True if occurrence of apply errors should
disable the subscription"

Regards,
Greg Nancarrow
Fujitsu Australia

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 16 Dec 2021 12:51:04 +0000

On Thursday, December 16, 2021 2:32 PM Greg Nancarrow <[email-masked]> wrote:
> > Besides all of those changes, I've removed the obsolete comment of
> > DisableSubscriptionOnError in v12.
> >
> 

> However, in create_subscription.sgml, it says "disabled if any errors are
> detected by subscription workers ..."
> 
Okay. Fixed.
 
Agreed. Fixed.

> The comment should just say "True if occurrence of apply errors should disable
> the subscription"
Fixed.

Attached the updated patch v14.

Best Regards,
	Takamichi Osumi

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 21 Dec 2021 14:17:31 +0000

> Attached the updated patch v14.
FYI, I've conducted a test of disable_on_error flag using
pg_upgrade.  I prepared PG14 and HEAD applied with disable_on_error patch.
Then, I setup a logical replication pair of the publisher and the subscriber by 14
and executed pg_upgrade for both the publisher and the subscriber individually.

After the updation, on the subscriber, I've confirmed the disable_on_error is false
via both pg_subscription and \dRs+, as expected.

Best Regards,
	Takamichi Osumi

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 22 Dec 2021 10:24:05 +0000

On Tuesday, December 21, 2021 11:18 PM I wrote:
> FYI, I've conducted a test of disable_on_error flag using pg_upgrade.  I
> prepared PG14 and HEAD applied with disable_on_error patch.
> After the updation, on the subscriber, I've confirmed the disable_on_error is
> false via both pg_subscription and \dRs+, as expected.
Additionally, I've tested the new TAP test in a tight loop
that executed 027_disable_on_error.pl 100 times sequentially.
There was no failure, which means
any timing issue should not exist in the test.

Best Regards,
	Takamichi Osumi

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 28 Dec 2021 02:52:47 +0000

> Attached the updated patch v14.

A comment to the timing of printing a log:
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?

Regards,
Wang wei