Hi,
On 2022-11-28 11:42:19 +0530, Bharath Rupireddy wrote:
> On Sun, Nov 27, 2022 at 2:43 AM Andres Freund <andres@anarazel.de> wrote:
> > 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.
So what? It's far from the only operation doing so. And in contrast to most of
the other places (c.f. WALInsertLockAcquireExclusive()) it only takes one of
them at a time.
> 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.
A central atomic in XLogCtlInsert would be better than a spinlock protected
variable, but still bad. We do *not* want to have more central state that
needs to be manipulated, we want *less*.
If we wanted to optimize this - and I haven't seen any evidence it's worth
doing so - we should just optimize the lock acquisitions in
GetLastImportantRecPtr() away. *Without* centralizing the state.
Greetings,
Andres Freund
