On Wed, Jan 19, 2022 at 12:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Jan 19, 2022 at 12:22 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> >
> > (6) apply_handle_stream_abort
> >
> > @@ -1209,6 +1300,13 @@ apply_handle_stream_abort(StringInfo s)
> >
> > logicalrep_read_stream_abort(s, &xid, &subxid);
> >
> > + /*
> > + * We don't expect the user to set the XID of the transaction that is
> > + * rolled back but if the skip XID is set, clear it.
> > + */
> > + if (MySubscription->skipxid == xid || MySubscription->skipxid == subxid)
> > + clear_subscription_skip_xid(MySubscription->skipxid, InvalidXLogRecPtr, 0);
> > +
> >
> > In my humble opinion, this still cares about subtransaction xid still.
> > If we want to be consistent with top level transactions only,
> > I felt checking MySubscription->skipxid == xid should be sufficient.
>
> I thought if we can clear subskipxid whose value has already been
> processed on the subscriber with a reasonable cost it makes sense to
> do that because it can reduce the possibility of the issue that XID is
> wraparound while leaving the wrong in subskipxid.
>
I guess that could happen if the user sets some unrelated XID value.
So, I think it should be okay to not clear this but we can add a
comment in the code at that place that we don't clear subtransaction's
XID as we don't support skipping individual subtransactions or
something like that.
--
With Regards,
Amit Kapila.