Re: Skipping logical replication transactions on subscriber side

On Tue, Nov 2, 2021 at 2:35 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Nov 1, 2021 at 7:18 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Fri, Oct 29, 2021 at 8:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > >
> > > Fair enough. So statistics can be removed either by vacuum or drop
> > > subscription. Also, if we go by this logic then there is no harm in
> > > retaining the stat entries for tablesync errors. Why have different
> > > behavior for apply and tablesync workers?
> >
> > My understanding is that the subscription worker statistics entry
> > corresponds to workers (but not physical workers since the physical
> > process is changed after restarting). So if the worker finishes its
> > jobs, it is no longer necessary to show errors since further problems
> > will not occur after that. Table sync worker’s job finishes when
> > completing table copy (unless table sync is performed again by REFRESH
> > PUBLICATION) whereas apply worker’s job finishes when the subscription
> > is dropped.
> >
>
> Actually, I am not very sure how users can use the old error
> information after we allowed skipping the conflicting xid. Say, if
> they want to add/remove some constraints on the table based on
> previous errors then they might want to refer to errors of both the
> apply worker and table sync worker.

I think that in general, statistics should be retained as long as a
corresponding object exists on the database, like other cumulative
statistic views. So I’m concerned that an entry of a cumulative stats
view is automatically removed by a non-stats-related function (i.g.,
ALTER SUBSCRIPTION SKIP). Which seems a new behavior for cumulative
stats views.

We can retain the stats entries for table sync worker but what I want
to avoid is that the view shows many old entries that will never be
updated. I've sometimes seen cases where the user mistakenly restored
table data on the subscriber before creating a subscription, failed
table sync on many tables due to unique violation, and truncated
tables on the subscriber. I think that unlike the stats entries for
apply worker, retaining the stats entries for table sync could be
harmful since it’s likely to be a large amount (even hundreds of
entries). Especially, it could lead to bloat the stats file since it
has an error message. So if we do that, I'd like to provide a function
for users to remove (not reset) stats entries manually. Even if we
removed stats entries after skipping the transaction in question, the
stats entries would be left if we resolve the conflict in another way.

>
> > >
> > > I have another question in this regard. Currently, the reset function
> > > seems to be resetting only the first stat entry for a subscription.
> > > But can't we have multiple stat entries for a subscription considering
> > > the view's cumulative nature?
> >
> > I might be missing your points but I think that with the current
> > patch, the view has multiple entries for a subscription. That is,
> > there is one apply worker stats and multiple table sync worker stats
> > per subscription.
> >
>
> Can't we have multiple entries for one apply worker?

Umm, I think we have one stats entry per one logical replication
worker (apply worker or table sync worker). Am I missing something?

>
> > And pg_stat_reset_subscription() function can reset
> > any stats by specifying subscription OID and relation OID.
> >
>
> Say, if the user has supplied just subscription OID then isn't it
> better to reset all the error entries for that subscription?

Agreed. So pg_stat_reset_subscription_worker(oid) removes all errors
for the subscription whereas pg_stat_reset_subscription_worker(oid,
null) reset only the apply worker error for the subscription?

Regards,

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