Re: Skipping logical replication transactions on subscriber side

On Tue, Jul 6, 2021 at 6:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jul 6, 2021 at 12:30 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Jul 5, 2021 at 6:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Jul 1, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Thu, Jul 1, 2021 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > >
> > > > > Don't we want to clear stats at drop subscription as well? We do drop
> > > > > database stats in dropdb via pgstat_drop_database, so I think we need
> > > > > to clear subscription stats at the time of drop subscription.
> > > >
> > > > Yes, it needs to be cleared. In the 0003 patch, pgstat_vacuum_stat()
> > > > sends the message to clear the stats. I think it's better to have
> > > > pgstat_vacuum_stat() do that job similar to dropping replication slot
> > > > statistics rather than relying on the single message send at DROP
> > > > SUBSCRIPTION. I've considered doing both: sending the message at DROP
> > > > SUBSCRIPTION and periodical checking by pgstat_vacuum_stat(), but
> > > > dropping subscription not setting a replication slot is able to
> > > > rollback. So we need to send it only at commit time. Given that we
> > > > don’t necessarily need the stats to be updated immediately, I think
> > > > it’s reasonable to go with only a way of pgstat_vacuum_stat().
> > > >
> > >
> > > Okay, that makes sense. Can we consider sending the multiple ids in
> > > one message as we do for relations or functions in
> > > pgstat_vacuum_stat()? That will reduce some message traffic.
> >
> > Yes. Since subscriptions are objects that are not frequently created
> > and dropped I prioritized not to increase the message type. But if we
> > do that for subscriptions, is it better to do that for replication
> > slots as well? It seems to me that the lifetime of subscriptions and
> > replication slots are similar.
> >
>
> Yeah, I think it makes sense to do for both, we can work on slots
> patch separately. I don't see a reason why we shouldn't send a single
> message for multiple clear/drop entries.

+1

>
> > >
> > > True but I guess the user can wait for all the tablesyncs to either
> > > finish or get an error corresponding to the table sync. After that, it
> > > can use 'copy_data' as false. This is not a very good method but I
> > > don't see any other option. I guess whatever is the case logging
> > > errors from tablesyncs is anyway not a bad idea.
> > >
> > > Instead of using the syntax "ALTER SUBSCRIPTION name SET SKIP
> > > TRANSACTION Iconst", isn't it better to use it as a subscription
> > > option like Mark has done for his patch (disable_on_error)?
> >
> > According to the doc, ALTER SUBSCRIPTION ... SET is used to alter
> > parameters originally set by CREATE SUBSCRIPTION. Therefore, we can
> > specify a subset of parameters that can be specified by CREATE
> > SUBSCRIPTION. It makes sense to me for 'disable_on_error' since it can
> > be specified by CREATE SUBSCRIPTION. Whereas SKIP TRANSACTION stuff
> > cannot be done. Are you concerned about adding a syntax to ALTER
> > SUBSCRIPTION?
> >
>
> Both for additional syntax and consistency with disable_on_error.
> Isn't it just a current implementation that Alter only allows to
> change parameters supported by Create? Is there a reason why we can't
> allow Alter to set/change some parameters not supported by Create?

I think there is not reason for that but looking at ALTER TABLE I
thought there is such a policy. I thought the skipping transaction
feature is somewhat different from disable_on_error feature. The
former seems a feature to deal with a problem on the spot whereas the
latter seems a setting of a subscription. Anyway, if we use the
subscription option, we can reset the XID by setting 0? Or do we need
ALTER SUBSCRIPTION RESET?

Regards,

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