public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
To: Zhijie Hou (Fujitsu) <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Aleksander Alekseev <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: shveta malik <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Jan Wieck <[email protected]>
Subject: Re: Commit Timestamp and LSN Inversion issue
Date: Fri, 8 Nov 2024 09:53:01 -0500
Message-ID: <xhcapr7lur35itqrbk4qdqn7rzfaqr3g6dvyxzfx6jeyknhvbv@h3wkgixuuuon> (raw)
In-Reply-To: <OS0PR01MB57162A227EC357482FEB4470945D2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
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>
<[email protected]>
<gkbrjxwyjbqckjbrkep3uy2mvrcq2mrfzprfmegbeiqxidtmbu@3cgs5iocjykk>
<OS0PR01MB57162A227EC357482FEB4470945D2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
Hi,
On 2024-11-08 09:08:55 +0000, Zhijie Hou (Fujitsu) wrote:
> On Friday, November 8, 2024 2:20 AM Andres Freund <[email protected]> wrote:
> > On 2024-11-05 08:58:36 -0500, Jan Wieck wrote:
> > > The attached solution is minimally invasive because it doesn't move
> > > the timestamp generation (clock_gettime() call) into the critical
> > > section of
> > > ReserveXLogInsertLocation() that is protected by a spinlock. Instead
> > > it keeps track of the last commit-ts written to WAL in shared memory
> > > and simply bumps that by one microsecond if the next one is below or
> > > equal. There is one extra condition in that code section plus a
> > > function call by pointer for every WAL record. In the unlikely case of
> > > encountering a stalled or backwards running commit-ts, the expensive
> > > part of recalculating the CRC of the altered commit WAL-record is done
> > > later, after releasing the spinlock. I have not been able to measure
> > > any performance impact on a machine with 2x Xeon-Silver (32 HT cores).
> >
> > I think it's *completely* unacceptable to call a hook inside
> > ReserveXLogInsertLocation, with the spinlock held, no less. That's the 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 commit
> timestamp within the spinlock, aiming to keep it as efficient as possible. 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 nontrivial
instruction to the locked portion of ReserveXLogInsertLocation().
Before long we're going to have to replace that spinlocked instruction with
atomics, which will make it impossible to just block while holding the lock
anyway.
IMO nothing that touches core xlog insert infrastructure is acceptable for
this.
Greetings,
Andres Freund
view thread (33+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Commit Timestamp and LSN Inversion issue
In-Reply-To: <xhcapr7lur35itqrbk4qdqn7rzfaqr3g6dvyxzfx6jeyknhvbv@h3wkgixuuuon>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox