On Wed, Jan 19, 2022 at 5:58 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.
Agreed and added the comment in the latest patch[1].
Regards,
[1] https://www.postgresql.org/message-id/CAD21AoDOuNtvFUfU2wH2QgTJ6AyMXXh_vdA87qX0mUibdsrYTg%40mail.gmail.com
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/