On Tue, Jul 6, 2021 at 6:33 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Jul 6, 2021 at 12:30 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > On Mon, Jul 5, 2021 at 6:46 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Thu, Jul 1, 2021 at 6:31 PM Masahiko Sawada <sawada.m...@gmail.com> > > > wrote: > > > > > > > > On Thu, Jul 1, 2021 at 12:56 PM Amit Kapila <amit.kapil...@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/
