Re: Skipping logical replication transactions on subscriber side

On Tue, Jan 11, 2022 at 8:52 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Jan 10, 2022 at 8:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > I was thinking what if we don't advance origin explicitly in this
> > case? Actually, that will be no different than the transactions where
> > the apply worker doesn't apply any change because the initial sync is
> > in progress (see should_apply_changes_for_rel()) or we have received
> > an empty transaction. In those cases also, the origin lsn won't be
> > advanced even though we acknowledge the advanced last_received
> > location because of keep_alive messages. Now, it is possible after the
> > restart we send the old start_lsn location because the replication
> > origin was not updated before restart but we handle that case in the
> > server by starting from the last confirmed location. See below code:
> >
> > CreateDecodingContext()
> > {
> > ..
> > else if (start_lsn < slot->data.confirmed_flush)
> > ..
>
> Good point. Probably one minor thing that is different from the
> transaction where the apply worker applied an empty transaction is a
> case where the server restarts/crashes before sending an
> acknowledgment of the flush location. That is, in the case of the
> empty transaction, the publisher sends an empty transaction again. On
> the other hand in the case of skipping the transaction, a non-empty
> transaction will be sent again but skip_xid is already changed or
> cleared, therefore the user will have to specify skip_xid again. If we
> write replication origin WAL record to advance the origin lsn, it
> reduces the possibility of that. But I think it’s a very minor case so
> we won’t need to deal with that.
>

Yeah, in the worst case, it will lead to conflict again and the user
needs to set the xid again.

> Anyway, according to your analysis, I think we don't necessarily need
> to do replorigin_advance() in this case.
>

Right.

> >
> > 5.
> > +          by the logical replication worker. Setting
> > <literal>NONE</literal> means
> > +          to reset the transaction ID.
> >
> > Let's slightly reword the second part of the sentence as: "Setting
> > <literal>NONE</literal> resets the transaction ID."
>
> Will fix.
>
> >
> > 6.
> > Once we start skipping
> > + * changes, we don't stop it until the we skip all changes of the
> > transaction even
> > + * if the subscription invalidated and MySubscription->skipxid gets
> > changed or reset.
> >
> > /subscription invalidated/subscription is invalidated
>
> Will fix.
>
> >
> > What do you mean by subscription invalidated and how is it related to
> > this feature? I think we should mention something on these lines in
> > the docs as well.
>
> I meant that MySubscription, a cache of pg_subscription entry, is
> invalidated by the catalog change. IIUC while applying changes we
> don't re-read pg_subscription (i.e., not calling
> maybe_reread_subscription()). Similarly, while skipping changes, we
> also don't do that. Therefore, even if skip_xid has been changed while
> skipping changes, we don't stop skipping changes.
>

Okay, but I don't think we need to mention subscription is invalidated
as that could be confusing, the other part of the comment is quite
clear.

-- 
With Regards,
Amit Kapila.