On Thu, Oct 28, 2021 at 7:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 28, 2021 at 10:36 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Oct 27, 2021 at 7:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Oct 21, 2021 at 10:29 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > >
> > > > I've attached updated patches.
> >
> > Thank you for the comments!
> >
> > >
> > > Few comments:
> > > ==============
> > > 1. Is the patch cleaning tablesync error entries except via vacuum? If
> > > not, can't we send a message to remove tablesync errors once tablesync
> > > is successful (say when we reset skip_xid or when tablesync is
> > > finished) or when we drop subscription? I think the same applies to
> > > apply worker. I think we may want to track it in some way whether an
> > > error has occurred before sending the message but relying completely
> > > on a vacuum might be the recipe of bloat. I think in the case of a
> > > drop subscription we can simply send the message as that is not a
> > > frequent operation. I might be missing something here because in the
> > > tests after drop subscription you are expecting the entries from the
> > > view to get cleared
> >
> > Yes, I think we can have tablesync worker send a message to drop stats
> > once tablesync is successful. But if we do that also when dropping a
> > subscription, I think we need to do that only the transaction is
> > committed since we can drop a subscription that doesn't have a
> > replication slot and rollback the transaction. Probably we can send
> > the message only when the subscritpion does have a replication slot.
> >
>
> Right. And probably for apply worker after updating skip xid.
I'm not sure it's better to drop apply worker stats after resetting
skip xid (i.g., after skipping the transaction). Since the view is a
cumulative view and has last_error_time, I thought we can have the
apply worker stats until the subscription gets dropped. Since the
error reporting message could get lost, no entry in the view doesn’t
mean the worker doesn’t face an issue.
>
> > In other cases, we can remember the subscriptions being dropped and
> > send the message to drop the statistics of them after committing the
> > transaction but I’m not sure it’s worth having it.
> >
>
> Yeah, let's not go to that extent. I think in most cases subscriptions
> will have corresponding slots.
Agreed.
>
> FWIW, we completely
> > rely on pg_stat_vacuum_stats() for cleaning up the dead tables and
> > functions. And we don't expect there are many subscriptions on the
> > database.
> >
>
> True, but we do send it for the database, so let's do it for the cases
> you explained in the first paragraph.
Agreed.
I've attached a new version patch. Since the syntax of skipping
transaction id is under the discussion I've attached only the error
reporting patch for now.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
