Re: Commit Timestamp and LSN Inversion issue

Hi,

On 2024-11-08 09:08:55 +0000, Zhijie Hou (Fujitsu) wrote:
> On Friday, November 8, 2024 2:20 AM Andres Freund <andres(at)anarazel(dot)de> 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