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-17T14:34:31Z
Lists: pgsql-hackers 
On Tue, Jun 16, 2020 at 3:28 PM Andres Freund <andres@anarazel.de> wrote:
> > 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:
>
> My main motivation was to have something that runs more often than than
> the embeded test in s_lock.c's that nobody ever runs (they wouldn't even
> pass with disabled spinlocks, as S_LOCK_FREE isn't implemented).

Sure, that makes sense.

> > + /* 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.
>
> Hm? Isn't s_lock the, as its comment says, "platform-independent portion
> of waiting for a spinlock."?  I also don't think we need to purely
> follow external APIs in internal tests.

I feel like we at least didn't use to use that on all platforms, but I
might be misremembering. It seems odd and confusing that we have  both
S_LOCK() and s_lock(), anyway. Differentiating functions based on case
is not great practice.

> Sure, there's a lot that'd pass. But it's more than we had before. It
> did catch a bug much quicker than I'd have otherwise found it, FWIW.
>
> I don't think an empty implementation would pass btw, as long as TAS is
> defined.

Fair enough.

> Yea, we could use something better. But I don't see that happening
> quickly, and having something seems better than nothing.
>
> That seems quite hard to achieve. I really just wanted to have something
> I can do some very basic tests to catch issues quicker.
>
> The atomics tests found numerous issues btw, despite also not testing
> concurrency.
>
> I think we generally have way too few of such trivial tests. They can
> find plenty "real world" issues, but more importantly make it much
> quicker to iterate when working on some piece of code.
>
> Without the tests I couldn't even reproduce a deadlock due to the
> nesting. So they imo are pretty essential?

I'm not telling you not to commit these; I'm just more skeptical of
whether they are the right approach than you seem to be. But that's
OK: people can like different things, and I don't know exactly what
would be better anyway.

-- 
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.