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

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);

Best Regards,
Hayato Kuroda
FUJITSU LIMITED