Re: test_decoding assertion failure for the loss of top-sub transaction relationship

At Fri, 2 Sep 2022 11:27:23 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in 
> On Fri, Sep 2, 2022 at 11:25 AM kuroda.hayato@fujitsu.com
> <kuroda.hayato@fujitsu.com> 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. 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.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center