Horton

Re: global barrier & atomics in signal handlers (Re: Atomic operations within spinlocks)

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Tom Lane <tgl@sss.pgh.pa.us>
Date: 2020-06-16T18:59:19Z
Lists: pgsql-hackers 
On Mon, Jun 15, 2020 at 9:37 PM Andres Freund <andres@anarazel.de> wrote:
> What do you think about 0002?
>
> With regard to the cost of the expensive test in 0003, I'm somewhat
> inclined to add that to the buildfarm for a few days and see how it
> actually affects the few bf animals without atomics. We can rip it out
> after we got some additional coverage (or leave it in if it turns out to
> be cheap enough in comparison).

I looked over these patches briefly today. I don't have any objection
to 0001 or 0002. I think 0003 looks a little strange: it seems to be
testing things that might be implementation details of other things,
and I'm not sure that's really correct. In particular:

+ /* and that "contended" acquisition works */
+ s_lock(&struct_w_lock.lock, "testfile", 17, "testfunc");
+ S_UNLOCK(&struct_w_lock.lock);

I didn't think we had formally promised that s_lock() is actually
defined or working on all platforms.

More generally, I don't think it's entirely clear what all of these
tests are testing. Like, I can see that data_before and data_after are
intended to test that the lock actually fits in the space allowed for
it, but at the same time, I think empty implementations of all of
these functions would pass regression, as would many horribly or
subtly buggy implementations. For example, consider this:

+ /* test basic operations via the SpinLock* API */
+ SpinLockInit(&struct_w_lock.lock);
+ SpinLockAcquire(&struct_w_lock.lock);
+ SpinLockRelease(&struct_w_lock.lock);

What does it look like for this test to fail? I guess one of those
operations has to fail an assert or hang forever, because it's not
like we're checking the return value. So I feel like the intent of
these tests isn't entirely clear, and should probably be explained
better, at a minimum -- and perhaps we should think harder about what
a good testing framework would look like. I would rather have tests
that either pass or fail and report a result explicitly, rather than
tests that rely on hangs or crashes.

Parenthetically, "cyle" != "cycle".

I don't have any real complaints about the functionality of 0004 on a
quick read-through, but I'm again a bit skeptical of the tests. Not as
much as with 0003, though.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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.