Re: Optionally automatically disable logical replication subscriptions on error

On Mon, Jun 21, 2021 at 9:21 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Mon, Jun 21, 2021 at 12:09 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Jun 21, 2021 at 7:56 AM Mark Dilger
> > <mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> > >
> > > > On Jun 20, 2021, at 7:17 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > > I will submit the patch.
> > >
> > > Great, thanks!
> > >
> > > > 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.
> > >
> > > Right. Subscription on error triggers would need that, too, if we wrote them.
> > >
> > > > 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.
> > > >
> >
> > I think the two key points are (a) to define exactly what all
> > 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.
>

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.

> > (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.
>
> 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).
>

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.