Re: Skipping logical replication transactions on subscriber side

On Mon, Jan 17, 2022 at 2:48 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Jan 17, 2022 at 9:49 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Sat, Jan 15, 2022 at 7:24 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> >
> > > 6.
> > > +static void
> > > +maybe_start_skipping_changes(TransactionId xid)
> > > +{
> > > + Assert(!is_skipping_changes());
> > > + Assert(!in_remote_transaction);
> > > + Assert(!in_streamed_transaction);
> > > +
> > > + /* Make sure subscription cache is up-to-date */
> > > + maybe_reread_subscription();
> > >
> > > Why do we need to update the cache here by calling
> > > maybe_reread_subscription() and at other places in the patch? It is
> > > sufficient to get the skip_xid value at the start of the worker via
> > > GetSubscription().
> >
> > MySubscription could be out-of-date after a user changes the catalog.
> > In non-skipping change cases, we check it when starting the
> > transaction in begin_replication_step() which is called, e.g., when
> > applying an insert change. But I think we need to make sure it’s
> > up-to-date at the beginning of applying changes, that is, before
> > starting a transaction. Otherwise, we may end up skipping the
> > transaction based on out-of-dated subscription cache.
> >
>
> I thought the user would normally set skip_xid only after an error
> which means that the value should be as new as the time of the start
> of the worker. I am slightly worried about the cost we might need to
> pay for this additional look-up in case skip_xid is not changed. Do
> you see any valid user scenario where we might not see the required
> skip_xid? I am okay with calling this if we really need it.

Fair point. I've changed the code accordingly.

>
> > >
> > > 7. In maybe_reread_subscription(), isn't there a need to check whether
> > > skip_xid is changed where we exit and launch the worker and compare
> > > other subscription parameters?
> >
> > IIUC we relaunch the worker here when subscription parameters such as
> > slot_name was changed. In the current implementation, I think that
> > relaunching the worker is not necessarily necessary when skip_xid is
> > changed. For instance, when skipping the prepared transaction, we
> > deliberately don’t clear subskipxid of pg_subscription and do that at
> > commit-prepared or rollback-prepared case. There are chances that the
> > user changes skip_xid before commit-prepared or rollback-prepared. But
> > we tolerate this case.
> >
>
> I think between prepare and commit prepared, the user only needs to
> change it if there is another error in which case we will anyway
> restart and load the new value of same. But, I understand that we
> don't need to restart if skip_xid is changed as it might not impact
> remote connection in any way, so I am fine for not doing anything for
> this.

I'll leave this part for now. We can change it later if others think
it's necessary.

I've attached an updated patch. Please review it.

Regards,

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