On Fri, Nov 8, 2024 at 8:23 PM Andres Freund <andres@anarazel.de> wrote:
>
> 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 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.
>
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.
--
With Regards,
Amit Kapila.
