On Sun, Nov 27, 2022 at 2:43 AM Andres Freund <and...@anarazel.de> wrote: > > Hi, > > On 2022-11-23 19:12:07 +0530, Bharath Rupireddy wrote: > > While working on something else, I noticed that each WAL insert lock > > tracks its own last important WAL record's LSN (lastImportantAt) and > > both the bgwriter and checkpointer later computes the max > > value/server-wide last important WAL record's LSN via > > GetLastImportantRecPtr(). While doing so, each WAL insertion lock is > > acquired in exclusive mode in a for loop. This seems like too much > > overhead to me. > > GetLastImportantRecPtr() should be a very rare operation, so it's fine for it > to be expensive. The important thing is for the maintenance of the underlying > data to be very cheap. > > > I quickly coded a patch (attached herewith) that > > tracks the server-wide last important WAL record's LSN in > > XLogCtlInsert (lastImportantPos) protected with a spinlock and gets > > rid of lastImportantAt from each WAL insert lock. > > That strikes me as a very bad idea. It adds another point of contention to a > very very hot code path, to make a very rare code path cheaper.
Thanks for the response. I agree that GetLastImportantRecPtr() gets called rarely, however, what concerns me is that it's taking all the WAL insertion locks when it gets called. Is tracking lastImportantPos as pg_atomic_uint64 in XLogCtlInsert any better than an explicit spinlock? I think it's better on platforms where atomics are supported, however, it boils down to using a spin lock on the platforms where atomics aren't supported. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com