Re: Skipping logical replication transactions on subscriber side

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.

> BTW, do
> we have some way to avoid wrapping around the OID before we clean up
> via pgstat_vacuum_stat()?

As far as I know there is not.

>
>
> > > In the 0003 patch, if I am reading it correctly then the patch is not
> > > doing anything for tablesync worker. It is not clear to me at this
> > > stage what exactly we want to do about it? Do we want to just ignore
> > > errors from tablesync worker and let the system behave as it is
> > > without this feature? If we want to do anything then I think the way
> > > to skip the initial table sync would be to behave like the user has
> > > given 'copy_data' option as false.
> >
> > It might be better to have also sync workers report errors, even if
> > SKIP TRANSACTION feature doesn’t support anything for initial table
> > synchronization. From the user perspective, The initial table
> > synchronization is also the part of logical replication operations. If
> > we report only error information of applying logical changes, it could
> > confuse users.
> >
> > But I’m not sure about the way to skip the initial table
> > synchronization. Once we set `copy_data` to false, all table
> > synchronizations are disabled. Some of them might have been able to
> > synchronize successfully. It might be useful if the user can disable
> > the table initialization for the particular tables.
> >
>
> 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?

>
> I am slightly nervous about this way of allowing the user to skip the
> errors because if it is not used carefully then it can easily lead to
> inconsistent data on the subscriber. I agree that as only superusers
> will be allowed to use this option and we can document clearly the
> side-effects, the risk could be reduced but is that sufficient? It is
> not that we don't have any other tool which allows users to make their
> data inconsistent (one recent example is functions
> (heap_force_kill/heap_force_freeze) in pg_surgery module) if not used
> carefully but it might be better to not expose such tools.
>
> OTOH, if we use the error infrastructure of this patch and allow users
> to just disable the subscription on error as was proposed by Mark then
> that can't lead to any inconsistency.
>
> What do you think?

As you mentioned in another mail, what we can do with this feature is
the same as pg_replication_origin_advance(). Like there is a risk that
the user specifies a wrong LSN to pg_replication_origin_advance(),
there is a similar risk at this feature.

Regards,

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