On Mon, Jun 15, 2020 at 9:37 PM Andres Freund <and...@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
