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> <CAA4eK1KQ6H4u5=_5Xq44HhX3RFXW4HW-z0F4T4J1nf2xVCo04w@mail.gmail.com>
In-Reply-To: <CAA4eK1KQ6H4u5=_5Xq44HhX3RFXW4HW-z0F4T4J1nf2xVCo04w@mail.gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 12 Nov 2024 09:51:03 +0530
Message-ID: <CAA4eK1L6Vpf9EmL2bpfD3kdvM6_2XQqwJ9GY6uY-hgcot76cqA@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/CAA4eK1L6Vpf9EmL2bpfD3kdvM6_2XQqwJ9GY6uY-hgcot76cqA%40mail.gmail.com>
Precedence: bulk

On Tue, Nov 12, 2024 at 8:35=E2=80=AFAM Amit Kapila <amit.kapila16@gmail.co=
m> wrote:
>
> On Mon, Nov 11, 2024 at 9:05=E2=80=AFPM Tomas Vondra <tomas@vondra.me> wr=
ote:
> >
> > 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."
>

As the inversion issue can mainly hamper logical replication-based
solutions we can do any of this additional work under spinlock only
when the current record is a commit record (which the currently
proposed patch is already doing) and "wal_level =3D logical" and also
can have another option at the subscription level to enable this new
code path. I am not sure what is best but just sharing the ideas here.

--=20
With Regards,
Amit Kapila.