Horton

Re: Atomic operations within spinlocks

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org
Date: 2020-06-04T17:57:19Z
Lists: pgsql-hackers 
Andres Freund <andres@anarazel.de> writes:
> On 2020-06-03 14:19:45 -0400, Tom Lane wrote:
>> This seems to me to be very bad code.

> I think straight out prohibiting use of atomics inside a spinlock will
> lead to more complicated and slower code, rather than than improving
> anything. I'm to blame for the ClockSweepTick() case, and I don't really
> see how we could avoid the atomic-while-spinlocked without relying on
> 64bit atomics, which are emulated on more platforms, and without having
> a more complicated retry loop.

Well, if you don't want to touch freelist.c then there is no point
worrying about what pg_stat_get_wal_receiver is doing.  But having
now studied that freelist.c code, I don't like it one bit.  It's
overly complicated and not very accurately commented, making it
*really* hard to convince oneself that it's not buggy.

I think a good case could be made for ripping out what's there now
and just redefining nextVictimBuffer as a pg_atomic_uint64 that we
never reset (ie, make its comment actually true).  Then ClockSweepTick
reduces to

pg_atomic_fetch_add_u64(&StrategyControl->nextVictimBuffer, 1) % NBuffers

and when we want to know how many cycles have been completed, we just
divide the counter by NBuffers.  Now admittedly, this is relying on both
pg_atomic_fetch_add_u64 being fast and int64 division being fast ... but
to throw your own argument back at you, do we really care anymore about
performance on machines where those things aren't true?  Furthermore,
since all this is happening in a code path that's going to lead to I/O,
I'm not exactly convinced that a few nanoseconds matter anyway.

			regards, tom lane

Commits

  1. Convert SpinLock* macros to static inline functions.

  2. Clean up includes of s_lock.h.

  3. Fix deadlock danger when atomic ops are done under spinlock.

  4. Add basic spinlock tests to regression tests.

  5. spinlock emulation: Fix bug when more than INT_MAX spinlocks are initialized.

  6. Avoid potential spinlock in a signal handler as part of global barriers.

  7. Make pg_stat_wal_receiver consistent with the WAL receiver's shmem info

  8. Improve 64bit atomics support.