On Wed, Oct 19, 2022 at 11:58 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Tue, Oct 18, 2022 at 9:53 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > On Tue, Oct 18, 2022 at 7:49 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Tue, Oct 18, 2022 at 1:45 PM Masahiko Sawada <sawada.m...@gmail.com> > > > wrote: > > > > > > > > > > > > > > I think because the test case proposed needs all three changes, we can > > > > > push the change-1 without a test case and then as a second patch have > > > > > change-2 for HEAD and change-3 for back branches with the test case. > > > > > Do you have any other ideas to proceed here? > > > > > > > > I found another test case that causes the assertion failure at > > > > "Assert(!needs_snapshot || needs_timetravel);" on all branches. I've > > > > attached the patch for the test case. In this test case, I modified a > > > > user-catalog table instead of system-catalog table. That way, we don't > > > > generate invalidation messages while generating NEW_CID records. As a > > > > result, we mark only the subtransactions as containing catalog change > > > > and don't make association between top-level and sub transactions. The > > > > assertion failure happens on all supported branches. If we need to fix > > > > this (I believe so), Change-2 needs to be backpatched to all supported > > > > branches. > > > > > > > > There are three changes as Amit mentioned, and regarding the test > > > > case, we have three test cases I've attached: truncate_testcase.patch, > > > > analyze_testcase.patch, uesr_catalog_testcase.patch. The relationship > > > > between assertion failures and test cases are very complex. I could > > > > not find any test case to cause only one assertion failure on all > > > > branches. One idea to proceed is: > > > > > > > > Patch-1 includes Change-1 and is applied to all branches. > > > > > > > > Patch-2 includes Change-2 and the user_catalog test case, and is > > > > applied to all branches. > > > > > > > > Patch-3 includes Change-3 and the truncate test case (or the analyze > > > > test case), and is applied to v14 and v15 (also till v11 if we > > > > prefer). > > > > > > > > The patch-1 doesn't include any test case but the user_catalog test > > > > case can test both Change-1 and Change-2 on all branches. > > > > > > > > > > I was wondering if it makes sense to commit both Change-1 and Change-2 > > > together as one patch? Both assertions are caused by a single test > > > case and are related to the general problem that the association of > > > top and sub transaction is only guaranteed to be formed before we > > > decode transaction changes. Also, it would be good to fix the problem > > > with a test case that can cause it. What do you think? > > > > Yeah, it makes sense to me. > > > > I've attached two patches that need to be back-patched to all branches > and includes Change-1, Change-2, and a test case for them. FYI this > patch resolves the assertion failure reported in this thread as well > as one reported in another thread[2]. So I borrowed some of the > changes from the patch[2] Osumi-san recently proposed. >
Amit pointed out offlist that the changes in reorderbuffer.c is not pgindent'ed. I've run pgindent and attached updated patches. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
HEAD_v4-0001-Fix-the-assertion-failure-while-processing-NEW_CI.patch
Description: Binary data
v10-v15_v4-0001-Fix-the-assertion-failure-while-processing-NEW_CI.patch
Description: Binary data