Re: Optionally automatically disable logical replication subscriptions on error

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.

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

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/