MIME-Version: 1.0
References: <18369-ad61699bf91c5bc0@postgresql.org>
 <6d0e80d6.c1fc.18deeb8120a.Coremail.ocean_li_996@163.com>
 <TYCPR01MB120776F4E85D6066B2FE096C2F5222@TYCPR01MB12077.jpnprd01.prod.outlook.com>
 <45809022.60b5.18e11d304cf.Coremail.ocean_li_996@163.com>
In-Reply-To: <45809022.60b5.18e11d304cf.Coremail.ocean_li_996@163.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Fri, 8 Mar 2024 18:29:21 +0530
Message-ID: 
 <CAA4eK1KpW5pHMwMp9hfXYvOeEU5Rcbhoc7FxtBOGPgKeyYLDmA@mail.gmail.com>
Subject: Re: RE: Re:BUG #18369: logical decoding core on AssertTXNLsnOrder()
To: ocean_li_996 <ocean_li_996@163.com>
Cc: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
 Alexander Lakhin <exclusion@gmail.com>,
	"pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>,
 feichanghong@qq.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1KpW5pHMwMp9hfXYvOeEU5Rcbhoc7FxtBOGPgKeyYLDmA%40mail.gmail.com>
Precedence: bulk

On Wed, Mar 6, 2024 at 9:04=E2=80=AFAM ocean_li_996 <ocean_li_996@163.com> =
wrote:
>
> Thanks for your attention.
>
> At 2024-03-05 17:24:05, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.=
com> wrote:
>
> >Dear Haiyang Li,
> >
> >...
> >## Found issue
> >
> >### Empty transaction is decoded on PG14 and PG15
> >
> >However, there is a room for generating ReorderBufferTxn for empty trans=
actions,
> >which was introduced by 6b77048e5. Conditions are:
> >
> >1. There are sub transactions which modify only temp tables, and
> >2. the top transaction modifies the catalog.
> >
> >The call-stack toward the generation is below.
> >
> >```
> >ReorderBufferTXNByXid(create =3D true, create_as_top =3D true)
> >ReorderBufferXidSetCatalogChanges()    // for sub transactions
> >SnapBuildXidSetCatalogChanges()          // for top transaction
> >DecodeCommit()                                      // for top transacti=
on
> >```
> >
> >The path has been introduced by 6b77048e5.
> >Previously, calling ReorderBufferXidSetCatalogChanges() for sub transact=
ions
> >would be skipped, if they do not have catalog changes or they have not d=
ecoded yet.
> >However, the commit ensures sub transactions must be marked as containin=
g
> >catalog changes, and this also enforces to decode transactions even if i=
t is
> >empty.
> >
> >### Assertion failure
> >
> >The empty transactions would be created as top transactions. At that tim=
e,
> >AssertTXNLsnOrder() is called so that we ensured that first_lsn of top-t=
ransactions
> >must be strictly higher than previous. But they can be the same if there=
 are more
> >than two empty transactions. It led an assertion failure.
> >
> Your analysis is correct for me.  Actually, I mentioned in [1] that I can=
 reproduce this issue before 6b77048e5.
> After some attempts and analysis, I also believe that the issue will only=
 occur after 6b77048e5.
>
> > ...
> >
> >## Possible solutions
> >
> >I think there are several solutions.
> >Note that I assumed here that fixes for all the versions should be almos=
t the same.
> >
> >* Ease the condition in AssertTXNLsnOrder(). If the decoded transaction =
is empty,
> >  it can be allowed that the first_lsn is same as previous one.
> >  PSA file to see my consideration.
> LGFM.  For my observation, the most case failed on AsserTXNOrder is check=
ing empty
> decoded transaction. Maybe we should pay more attention to review Reorder=
BufferTXNIsEmpty.
>
> >* Generate a ReorderBufferTXN as sub transaction when we are in this pat=
h.
> >  The approach has already been shared by you. However, note that this n=
eeds to
> >  extend the ReorderBufferXidSetCatalogChanges function, and breaks ABI
> >  compatibility [1].
> Yes, It breaks ABI compatibility.
>

One possibility is introducing ReorderBufferXidSetCatalogChangesEx, a
new API with a bool parameter. I don't know if this is a good idea but
I prefer not to tinker with asserts as proposed by Kuroda-San in
another approach.

--=20
With Regards,
Amit Kapila.