Hi, On 2020-06-17 10:34:31 -0400, Robert Haas wrote: > On Tue, Jun 16, 2020 at 3:28 PM Andres Freund <and...@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: > > 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.
There's only one definition of S_LOCK, and s_lock is the only spinlock related user of perform_spin_delay(). So I don't think so? > It seems odd and confusing that we have both > S_LOCK() and s_lock(), anyway. Differentiating functions based on case > is not great practice. It's a terrible idea, yes. Since we don't actually have any non-default implementations of S_LOCK, perhaps we should just rip it out? It'd probably be clearer if SpinLockAcquire() would be what uses TAS() and falls back to s_lock (best renamed to s_lock_slowpath or such). It'd perhaps also be good to make SpinLockAcquire() a static inline instead of a #define, so it can be properly attributed in debuggers and profilers. Greetings, Andres Freund
