On Sun, Nov 27, 2022 at 2:43 AM Andres Freund <andres@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
