From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Subject: Atomic operations within spinlocks
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1141818.1591208385.1@sss.pgh.pa.us>
Date: Wed, 03 Jun 2020 14:19:45 -0400
Message-ID: <1141819.1591208385@sss.pgh.pa.us>
Precedence: bulk

In connection with the nearby thread about spinlock coding rule
violations, I noticed that we have several places that are doing
things like this:

	SpinLockAcquire(&WalRcv->mutex);
	...
	written_lsn = pg_atomic_read_u64(&WalRcv->writtenUpto);
	...
	SpinLockRelease(&WalRcv->mutex);

That's from pg_stat_get_wal_receiver(); there is similar code in
freelist.c's ClockSweepTick() and StrategySyncStart().

This seems to me to be very bad code.  In the first place, on machines
without the appropriate type of atomic operation, atomics.c is going
to be using a spinlock to emulate atomicity, which means this code
tries to take a spinlock while holding another one.  That's not okay,
either from the standpoint of performance or error-safety.  In the
second place, this coding seems to me to indicate serious confusion
about which lock is protecting what.  In the above example, if
writtenUpto is only accessed through atomic operations then it seems
like we could just move the pg_atomic_read_u64 out of the spinlock
section; or if the spinlock is adequate protection then we could just
do a normal fetch.  If we actually need both locks then this needs
significant re-thinking, IMO.

Comments?

			regards, tom lane