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

On Wed, Sep 7, 2022 at 11:06 AM kuroda(dot)hayato(at)fujitsu(dot)com
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Amit,
>
> Thanks for giving comments!
>
> > Did you get this new assertion failure after you applied the patch for
> > the first failure? Because otherwise, how can you reach it with the
> > same test case?
>
> The first failure is occurred only in the HEAD, so I did not applied the first patch
> to REL14 and REL15.
> This difference is caused because the commit [Fix catalog lookup...] in REL15(272248a) and older is different
> from the HEAD one.
> In order versions SnapBuildXidSetCatalogChanges() has been added. In the function
> a transaction will be marked as containing catalog changes if the transaction is in InitialRunningXacts,
> and after that the relation between sub-top transactions is assigned based on the parsed->subxact.
> The marking avoids the first failure, but the assignment triggers new failure.
>
>
> > About patch:
> > else if (sub_needs_timetravel)
> > {
> > - /* track toplevel txn as well, subxact alone isn't meaningful */
> > + elog(DEBUG2, "forced transaction %u to do timetravel due to one of
> > its subtransaction",
> > + xid);
> > + needs_timetravel = true;
> > SnapBuildAddCommittedTxn(builder, xid);
> >
> > Why did you remove the above comment? I think it still makes sense to retain it.
>
> Fixed.

Here are some review comments for v2 patch:

+# Test that we can force the top transaction to do timetravel when one of sub
+# transactions needs that. This is necessary when we restart decoding
from RUNNING_XACT
+# without the wal to associate subtransaction to its top transaction.

I don't think the second sentence is necessary.

---
The last decoding
+# starts from the first checkpoint and NEW_CID of "s0_truncate"
doesn't mark the top
+# transaction as catalog modifying transaction. In this scenario, the
enforcement sets
+# needs_timetravel to true even if the top transaction is regarded as
that it does not
+# have catalog changes and thus the decoding works without a
contradition that one
+# subtransaction needed timetravel while its top transaction didn't.

I don't understand the last sentence, probably it's a long sentence.

How about the following description?

# Test that we can handle the case where only subtransaction is marked
as containing
# catalog changes. The last decoding starts from NEW_CID generated by
"s0_truncate" and
# marks only the subtransaction as containing catalog changes but we
don't create the
# association between top-level transaction and subtransaction yet.
When decoding the
# commit record of the top-level transaction, we must force the
top-level transaction
# to do timetravel since one of its subtransactions is marked as
containing catalog changes.

---
+ elog(DEBUG2, "forced transaction %u to do timetravel due to one of
its subtransaction",
+ xid);
+ needs_timetravel = true;

I think "one of its subtransaction" should be "one of its subtransactions".

Regards,

--
Masahiko Sawada