Re: subscription disable_on_error not working after ALTER SUBSCRIPTION set bad conninfo

On Fri, Jan 19, 2024 at 7:21 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Thu, Jan 18, 2024 at 6:54 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Thu, Jan 18, 2024 at 11:15 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Jan 18, 2024 at 12:55 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > ...
> > > >
> > > > Although we can improve it to handle this case too, I'm not sure it's
> > > > a bug. The doc says[1]:
> > > >
> > > > Specifies whether the subscription should be automatically disabled if
> > > > any errors are detected by subscription workers during data
> > > > replication from the publisher.
> > > >
> > > > When an apply worker is trying to establish a connection, it's not
> > > > replicating data from the publisher.
> > > >
> > > > Regards,
> > > >
> > > > [1] https://www.postgresql.org/docs/devel/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-PARAMS-WITH-DISABLE-ON-ERROR
> > > >
> > > > --
> > > > Masahiko Sawada
> > > > Amazon Web Services: https://aws.amazon.com
> > >
> > > Yeah, I had also seen that wording of those docs. And I agree it
> > > leaves open some room for doubts because strictly from that wording it
> > > can be interpreted that establishing the connection is not actually
> > > "data replication from the publisher" in which case maybe there is no
> > > bug.
> > >
> >
> > As far as I remember that was the intention. The idea was if there is
> > any conflict during apply that users manually need to fix, they have
> > the provision to stop repeating the error. If we wish to extend the
> > purpose of this option for another valid use case and there is a good
> > way to achieve the same then we can discuss but I don't think we need
> > to change it in back-branches.
>
> I agree not to change it in back-branches.
>
> What is the use case of extending disable_on_error?
>

The use-case is that with my user-hat on I had assumed
disable_on_error behaviour was as per the name implied so the
subscription would disable on getting (any) error. OTOH I agree, my
expectation is not exactly what the current docs say.

Also, I had thought the motivation for that option was to avoid having
infinite repeating errors that might be caused by the user or data.
e.g. a simple typo in the conninfo can cause this error and AFAIK the
ALTER will appear successful so the user won't know anything about it
unless they also check the logs. OTOH something like a connection
error may only be temporary (caused by a network issue?) and not
caused by a user typo at all, so I can see perhaps that is why
disable_on_error is OK to be excluding connection errors.

TBH I think there are pros and cons to doing nothing and leaving the
existing behaviour as-is or extending it -- I'm happy to go either
way.

Another idea is to leave behaviour unchanged, but add a note in the
docs like "Note: connection errors (e.g. specifying a bad conninfo
using ALTER SUBSCRIPTION) will not cause the subscription to become
disabled"

Thoughts?

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