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

On Fri, Sep 2, 2022 at 11:25 AM kuroda(dot)hayato(at)fujitsu(dot)com
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Horiguchi-san, Dilip,
>
> Thank you for replying!
>
> > > It seems that SnapBuildCommitTxn() is already taking care of adding
> > > the top transaction to the committed transaction if any subtransaction
> > > has the catalog changes, it has just missed setting the flag so I
> > > think just setting the flag like this should be sufficient no?
> >
> > Oops! That's right.
>
> Basically I agreed, but I was not sure the message "found top level transaction..."
> should be output or not. It may be useful even if one of sub transactions contains the change.
>
> 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.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com