On Wed, Sep 7, 2022 at 11:06 AM kuroda.hay...@fujitsu.com <kuroda.hay...@fujitsu.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