Re: Optionally automatically disable logical replication subscriptions on error

On Thu, Nov 18, 2021 at 12:52 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Thursday, November 18, 2021 2:08 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
> > A minor comment:
> Thanks for your comments !
>
> > 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.
> 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]

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