On Tue, Oct 18, 2022 at 6:29 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Oct 17, 2022 at 4:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> > IIUC, here you are speaking of three different changes. Change-1: Add
> > a check in AssertTXNLsnOrder() to skip assert checking till we reach
> > start_decoding_at. Change-2: Set needs_timetravel to true in one of
> > the else if branches in SnapBuildCommitTxn(). Change-3: Remove the
> > call to ReorderBufferAssignChild() from SnapBuildXidSetCatalogChanges
> > in PG-14/15 as that won't be required after Change-1.
>
> Yes.
>
> >
> > AFAIU, Change-1 is required till v10; Change-2 and Change-3 are
> > required in HEAD/v15/v14 to fix the problem.
>
> IIUC Change-2 is required in v16 and HEAD
>
Why are you referring v16 and HEAD separately?
> but not mandatory in v15 and
> v14. The reason why we need Change-2 is that there is a case where we
> mark only subtransactions as containing catalog change while not doing
> that for its top-level transaction. In v15 and v14, since we mark both
> subtransactions and top-level transaction in
> SnapBuildXidSetCatalogChanges() as containing catalog changes, we
> don't get the assertion failure at "Assert(!needs_snapshot ||
> needs_timetravel)".
>
> Regarding Change-3, it's required in v15 and v14 but not in HEAD and
> v16. Since we didn't add SnapBuildXidSetCatalogChanges() to v16 and
> HEAD, Change-3 cannot be applied to the two branches.
>
> > Now, the second and third
> > changes are not required in branches prior to v14 because we don't
> > record invalidations via XLOG_XACT_INVALIDATIONS record. However, if
> > we want, we can even back-patch Change-2 and Change-3 to keep the code
> > consistent or maybe just Change-3.
>
> Right. I don't think it's a good idea to back-patch Change-2 in
> branches prior to v14 as it's not a relevant issue.
>
Fair enough but then why to even backpatch it to v15 and v14?
> Regarding
> back-patching Change-3 to branches prior 14, I think it may be okay
> til v11, but I'd be hesitant for v10 as the final release comes in a
> month.
>
So to fix the issue in all branches, what we need to do is to
backpatch change-1: in all branches till v10, change-2: in HEAD, and
change-3: in V15 and V14. Additionally, we think, it is okay to
backpatch change-3 till v11 as it is mainly done to avoid the problem
fixed by change-1 and it makes code consistent in back branches.
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?
--
With Regards,
Amit Kapila.
