MIME-Version: 1.0
References: <CAJpy0uBxEJnabEp3JS=n9X19Vx2ZK3k5AR7N0h-cSMtOwYV3fA@mail.gmail.com>
 <CAJ7c6TNZY3idBDoGOaoS_fF6vJuBMjnDhZC4i5V0J58s2SJ3AA@mail.gmail.com>
 <CAA4eK1LOmtMpmJAxsmN0yXJb-Q9vi6KThDYXdpVGL_WZW5ZgvQ@mail.gmail.com>
 <CAJ7c6TOcQPcydC7HW=xPEDN6_Ec7iSTP9j=1VTAeG=haguOQHw@mail.gmail.com>
 <CAA4eK1KhAxe6=VTxa6xEbc22_VVBLsVuvRH8Wxws8i50gH=eqw@mail.gmail.com>
 <9f7f4def-7ea8-4ad3-83e7-a8cc9d18c58a@wi3ck.info> <gkbrjxwyjbqckjbrkep3uy2mvrcq2mrfzprfmegbeiqxidtmbu@3cgs5iocjykk>
 <OS0PR01MB57162A227EC357482FEB4470945D2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <xhcapr7lur35itqrbk4qdqn7rzfaqr3g6dvyxzfx6jeyknhvbv@h3wkgixuuuon>
In-Reply-To: <xhcapr7lur35itqrbk4qdqn7rzfaqr3g6dvyxzfx6jeyknhvbv@h3wkgixuuuon>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Mon, 11 Nov 2024 13:49:58 +0530
Message-ID: <CAA4eK1+ZdL=sC4sdUOQz+X9=7Z8Uxhkd0VqwcB9rgzXjvu=6yA@mail.gmail.com>
Subject: Re: Commit Timestamp and LSN Inversion issue
To: Andres Freund <andres@anarazel.de>
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Aleksander Alekseev <aleksander@timescale.com>, 
	pgsql-hackers <pgsql-hackers@postgresql.org>, shveta malik <shveta.malik@gmail.com>, 
	"tomas@vondra.me" <tomas@vondra.me>, Jan Wieck <jan@wi3ck.info>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1%2BZdL%3DsC4sdUOQz%2BX9%3D7Z8Uxhkd0VqwcB9rgzXjvu%3D6yA%40mail.gmail.com>
Precedence: bulk

On Fri, Nov 8, 2024 at 8:23=E2=80=AFPM Andres Freund <andres@anarazel.de> w=
rote:
>
> On 2024-11-08 09:08:55 +0000, Zhijie Hou (Fujitsu) wrote:
> > >
> > > I think it's *completely* unacceptable to call a hook inside
> > > ReserveXLogInsertLocation, with the spinlock held, no less. That's th=
e most
> > > contended section of code in postgres.
> >
> > I understand your concern and appreciate the feedback. I've made some
> > adjustments to the patch by directly placing the code to adjust the com=
mit
> > timestamp within the spinlock, aiming to keep it as efficient as possib=
le. The
> > changes have resulted in just a few extra lines. Would this approach be
> > acceptable to you ?
>
> No, not at all. I think it's basically not acceptable to add any nontrivi=
al
> instruction to the locked portion of ReserveXLogInsertLocation().
>
> Before long we're going to have to replace that spinlocked instruction wi=
th
> atomics, which will make it impossible to just block while holding the lo=
ck
> anyway.
>
> IMO nothing that touches core xlog insert infrastructure is acceptable fo=
r
> this.
>

I can't think of a solution other than the current proposal where we
do both the operations (reserve WAL space for commit and adjust
commit_timestamp, if required) together to resolve LSN<->Timestamp
invertion issue. Please let us know if you have any other ideas for
solving this. Even, if we want to change ReserveXLogInsertLocation to
make the locked portion an atomic operation, we can still do it for
records other than commit. Now, if we don't have any other solution
for LSN<->Timestamp inversion issue, changing the entire locked
portion to atomic will close the door to solving this problem forever
unless we have some other way to solve it which can make it difficult
to rely on commit_timestamps for certain scenarios.

--=20
With Regards,
Amit Kapila.