On Fri, Sep 2, 2022 at 12:25 PM Kyotaro Horiguchi
<[email protected]> wrote:
>
> At Fri, 2 Sep 2022 11:27:23 +0530, Dilip Kumar <[email protected]> wrote
> in
> > On Fri, Sep 2, 2022 at 11:25 AM [email protected]
> > <[email protected]> wrote:
> > > How about following?
> > >
> > > diff --git a/src/backend/replication/logical/snapbuild.c
> > > b/src/backend/replication/logical/snapbuild.c
> > > index bf72ad45ec..a630522907 100644
> > > --- a/src/backend/replication/logical/snapbuild.c
> > > +++ b/src/backend/replication/logical/snapbuild.c
> > > @@ -1086,8 +1086,17 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr
> > > lsn, TransactionId xid,
> > > }
> > > }
> > >
> > > - /* if top-level modified catalog, it'll need a snapshot */
> > > - if (SnapBuildXidHasCatalogChanges(builder, xid, xinfo))
> > > + /*
> > > + * if top-level or one of sub modified catalog, it'll need a
> > > snapshot.
> > > + *
> > > + * Normally the second check is not needed because the relation
> > > between
> > > + * top-sub transactions is tracked on the ReorderBuffer layer,
> > > and the top
> > > + * transaction is marked as containing catalog changes if its
> > > children are.
> > > + * But in some cases the relation may be missed, in which case
> > > only the sub
> > > + * transaction may be marked as containing catalog changes.
> > > + */
> > > + if (SnapBuildXidHasCatalogChanges(builder, xid, xinfo)
> > > + || sub_needs_timetravel)
> > > {
> > > elog(DEBUG2, "found top level transaction %u, with
> > > catalog changes",
> > > xid);
> > > @@ -1095,11 +1104,6 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr
> > > lsn, TransactionId xid,
> > > needs_timetravel = true;
> > > SnapBuildAddCommittedTxn(builder, xid);
> > > }
> > > - else if (sub_needs_timetravel)
> > > - {
> > > - /* track toplevel txn as well, subxact alone isn't
> > > meaningful */
> > > - SnapBuildAddCommittedTxn(builder, xid);
> > > - }
> > > else if (needs_timetravel)
> > > {
> > > elog(DEBUG2, "forced transaction %u to do timetravel",
> > > xid);
> >
> > Yeah, I am fine with this as well.
>
> I'm basically fine, too. But this is a bug that needs back-patching
> back to 10.
>
I have not verified but I think we need to backpatch this till 14
because prior to that in DecodeCommit, we use to set the top-level txn
as having catalog changes based on the if there are invalidation
messages in the commit record. So, in the current scenario shared by
Osumi-San, before SnapBuildCommitTxn(), the top-level txn will be
marked as having catalog changes.
> This change changes the condition for the DEBUG2 message.
> So we need to add an awkward if() condition for the DEBUG2 message.
> Looking that the messages have different debug-level, I doubt there
> have been a chance they are useful. If we remove the two DEBUGx
> messages, I'm fine with the change.
>
I think these DEBUG2 messages could be useful, so instead of removing
these, I suggest we should follow Dilip's proposed fix and maybe add a
new DEBUG2 message on the lines of (("forced transaction %u to do
timetravel due to one of its subtransaction", xid) in the else if
(sub_needs_timetravel) condition if we think that will be useful too
but I am fine leaving the addition of new DEBUG2 message.
--
With Regards,
Amit Kapila.
