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