On Thu, Mar 19, 2026 at 8:26 PM Sami Imseih <[email protected]> wrote: > I just realized v2 removed the test for dshash_find_or_insert_extended > with the NO_OOM flag. Corrected in v3.
You need to get rid of the bare blocks. If you happen to be using an AI to help write your code, I definitely suggest telling it to remember not to ever do that. This verifies that dshash_dump() doesn't crash, but not that it produces useful output. Whether that's worth the infrastructure, I don't know. But more generally, I wonder what people think about how much value this kind of thing has. With just the core regression test suite, we get coverage of 76.5% of the lines in dshash.c. Adding the isolation test suite and the test_dsm_registry test suite brings coverage up to 81.9% (334 of 408). The things that are not covered are: OOM cases, dshash_destroy(), dshash_dump(), concurrency case in resizing. delete_key_from_bucket() loop iterating more than once, delete_item_from_bucket() returning false. With the v3 patch, running just the test_dshash suite, coverage rises to 93.4% of lines (381 of 408). The omissions are: some OOM cases, dshash_delete_bucket() successfully deletes something, concurrency case in resizing, most of delete_key_from_bucket(), delete_item_from_bucket() iterating more than once or returning false. Combining your patch with the test suite previously mentioned, we get up to 97.1% coverage by lines (396 of 408). So the patch definitely has some value: it adds coverage of 60+ lines of code. On the other hand, it still feels to me like we'd be far more likely to notice new dshash bugs based on its existing usages than because of this. If I were modifying dshash, I wouldn't run this test suite first; I'd run the regression tests first. And the patch does have a cost: it's 334 lines of code that will have to be maintained going forward. I don't know if that's worth it. I feel like we're a lot more likely to break dshash behavior in some complicated concurrency scenario than we are to break it in a test like this. And, if somebody modifies dshash_destory() or dshash_dump(), they just need to test the code at least once before committing to get essentially the same benefit we'd get from this, which you would hope people would do anyway. None of this is to say that this patch is a horrible idea, but I'm also not entirely convinced that it is an excellent idea, so I would like to hear some other opinions. Of course, there's also the fact that this patch is quite similar to some other test_* modules that we already have, like test_dsa or test_bitmapset. So maybe those are good and this is also good. But I'm not sure. The good news is, if we do want this, it probably doesn't need much more work to be committable. Thanks, -- Robert Haas EDB: http://www.enterprisedb.com
