On Fri, Nov 8, 2024 at 8:23 PM Andres Freund <and...@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.
