Re: Skipping logical replication transactions on subscriber side

On Tue, Jul 6, 2021 at 12:30 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Mon, Jul 5, 2021 at 6:46 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Thu, Jul 1, 2021 at 6:31 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Jul 1, 2021 at 12:56 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

> >
> > 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?

--
With Regards,
Amit Kapila.