RE: Optionally automatically disable logical replication subscriptions on error

On Wednesday, March 9, 2022 8:22 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Mar 9, 2022 at 2:21 PM Masahiko Sawada
> <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Wed, Mar 9, 2022 at 4:33 PM osumi(dot)takamichi(at)fujitsu(dot)com
> > <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> > >
> > > On Tuesday, March 8, 2022 10:23 PM Amit Kapila
> <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > On Tue, Mar 8, 2022 at 1:37 PM osumi(dot)takamichi(at)fujitsu(dot)com
> > > > <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> > > > >
> > >
> > >
> > > > 2. Is there a reason the patch doesn't allow workers to restart
> > > > via
> > > > maybe_reread_subscription() when this new option is changed, if
> > > > so, then let's add a comment for the same? We currently seem to be
> > > > restarting the worker on any change via Alter Subscription. If we
> > > > decide to change it for this option as well then I think we need
> > > > to accordingly update the current comment: "Exit if any parameter
> > > > that affects the remote connection was changed." to something like
> > > > "Exit if any parameter that affects the remote connection or a
> subscription option was changed..."
> > > I thought it's ok without the change at the beginning, but I was wrong.
> > > To make this new option aligned with others, I should add one check
> > > for this feature. Fixed.
> >
> > Why do we need to restart the apply worker when disable_on_error is
> > changed? It doesn't affect the remote connection at all. I think it
> > can be changed without restarting like synchronous_commit option.
> >
>
> oh right, I thought that how will we update its value in MySubscription after a
> change but as we re-read the pg_subscription table for the current
> subscription and update MySubscription, I feel we don't need to restart it. I
> haven't tested it but it should work without a restart.
Hi, attached v32 removed my additional code for maybe_reread_subscription.

Also, I judged that we don't need to add a comment for this feature in this patch.
It's because we can interpret this discussion from existing comments and codes.
(1) "Reread subscription info if needed. Most changes will be exit."
There are some cases we don't exit.
(2) Like "Exit if any parameter that affects the remote connection was changed.",
readers can understand no exit case matches the disable_on_error option change.

Kindly review the v32.

Best Regards,
Takamichi Osumi