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