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>
 <CAA4eK1+ZdL=sC4sdUOQz+X9=7Z8Uxhkd0VqwcB9rgzXjvu=6yA@mail.gmail.com> <11ee3489-40ba-45d6-ba6a-7c3def725e84@vondra.me>
In-Reply-To: <11ee3489-40ba-45d6-ba6a-7c3def725e84@vondra.me>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 12 Nov 2024 08:35:25 +0530
Message-ID: <CAA4eK1KQ6H4u5=_5Xq44HhX3RFXW4HW-z0F4T4J1nf2xVCo04w@mail.gmail.com>
Subject: Re: Commit Timestamp and LSN Inversion issue
To: Tomas Vondra <tomas@vondra.me>
Cc: Andres Freund <andres@anarazel.de>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, 
	Aleksander Alekseev <aleksander@timescale.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, 
	shveta malik <shveta.malik@gmail.com>, 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/CAA4eK1KQ6H4u5%3D_5Xq44HhX3RFXW4HW-z0F4T4J1nf2xVCo04w%40mail.gmail.com>
Precedence: bulk

On Mon, Nov 11, 2024 at 9:05=E2=80=AFPM Tomas Vondra <tomas@vondra.me> wrot=
e:
>
> On 11/11/24 09:19, Amit Kapila wrote:
> >
> > 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.
> >
>
> I don't know what the solution is, isn't the problem that
>
> (a) we record both values (LSN and timestamp) during commit
>
> (b) reading timestamp from system clock can be quite expensive
>
> It seems to me that if either of those two issues disappeared, we
> wouldn't have such an issue.
>
> For example, imagine getting a timestamp is super cheap - just reading
> and updating a simple counter from shmem, just like we do for the LSN.
> Wouldn't that solve the problem?
>

Yeah, this is exactly what I thought.

> For example, let's say XLogCtlInsert has two fields
>
>     int64 CommitTimestamp;
>
> and that ReserveXLogInsertLocation() also does this for each commit:
>
>     commit_timestamp =3D Insert->commit_timestamp++;
>
> while holding the insertpos_lock. Now we have the LSN and timestamp
> perfectly correlated.
>

Right, and the patch sent by Hou-San [1] (based on the original patch
by Jan) is somewhat on these lines. The idea you have shared or
implemented by the patch is a logical clock stored in shared memory.
So, what the patch does is that if the time recorded by the current
commit record is lesser than or equal to the logical clock (which
means after we record time in the commit code path and before we
reserve the LSN, there happens a concurrent transaction), we shall
increment the value of logical clock by one and use that as commit
time.

So, in most cases, we need to perform one additional "if check" and
"an assignment" under spinlock, and that too only for the commit
record. In some cases, when inversion happens, there will be "one
increment" and "two assignments."

> Of course, this timestamp would be completely
> detached from "normal" timestamps, it'd just be a sequence. But we could
> also once in a while "set" the timestamp to GetCurrentTimestamp(), or
> something like that. For example, there could be a "ticker" process,
> doing that every 1000ms, or 100ms, or whatever we think is enough.
>
> Or maybe it could be driven by the commit itself, say when some timeout
> expires, we assign too many items since the last commit, ...
>

If we follow the patch, we don't need this additional stuff. Isn't
what is proposed [1] quite similar to your idea? If so, we can save
this extra maintenance of commit timestamps.

[1] - https://www.postgresql.org/message-id/OS0PR01MB57162A227EC357482FEB44=
70945D2%40OS0PR01MB5716.jpnprd01.prod.outlook.com

With Regards,
Amit Kapila.