Re: Skipping logical replication transactions on subscriber side

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. BTW, do
we have some way to avoid wrapping around the OID before we clean up
via pgstat_vacuum_stat()?

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

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?

--
With Regards,
Amit Kapila.