On Fri, Dec 29, 2023 at 9:15 AM David Rowley <dgrowle...@gmail.com> wrote:
> I looked into this a bit more and I just can't see why the current > code feels like it must do the reallocation in functions such as > bms_del_members(). We don't repalloc the set there, ever, so why do > we need to do it when building with REALLOCATE_BITMAPSETS? It seems > to me the point of REALLOCATE_BITMAPSETS is to ensure that *if* we > possibly could end up reallocating the set that we *do* reallocate. I think the argument here is whether the REALLOCATE_BITMAPSETS option is intended to force a reallocation for every modification of a bitmapset, or only for modifications that could potentially require the set to be reallocated. IIUC, the v2 patch addresses the latter scenario. I agree that it can help find bugs in cases where there's more than one reference to a set, and a modification that could reallocate the bitmapset might leave the other references being dangling pointers. It seems to me that the former scenario also makes sense in some cases. For instance, let's say there are two pointers in two structs, s1->p and s2->p, both referencing the same bitmapset. If we modify the bitmapset via s1->p (even if no reallocation could occur), s2 would see different content via its pointer 'p'. Sometimes this is just wrong and could cause problems. If we can force a reallocation for every modification of the bitmapset, it'd be much easier to find such bugs. Having said that, I think the codes checked in by 71a3e8c43b and 7d58f2342b are far from perfect. And I agree that the bms_copy_and_free in v2 patch is a good idea, as well as the bms_is_valid_set. Thanks Richard