Re: Optionally automatically disable logical replication subscriptions on error

On Thu, Nov 18, 2021 at 12:52 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Thursday, November 18, 2021 2:08 PM Greg Nancarrow <gregn4422(at)gmail(dot)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