On Sat, 4 Oct 2025 at 02:29, Greg Burd <[email protected]> wrote: > > On Oct 3 2025, at 5:36 am, David Rowley <[email protected]> wrote: > > However, we know that having multiple pointers pointing to the same > > set is "Trouble" as there can be a repalloc() when the set is modified > > and needs more Bitmapwords. That would cause issues unless the code > > was always careful to assign the modified set to all pointers. > > Since the call sites I've mentioned don't assign the result of > > bms_union() to multiple pointers, I think the attached patch is safe. > > I think we're safe here as I'd imagine buildfarm animals or local tests > run with REALLOCATE_BITMAPSETS would point out these mistakes as > failures, no?
That does have the ability to catch many cases where Bitmapsets are unintentionally pointed to by multiple pointers and one set gets modified in-place without the new set being assigned to all pointers which are meant to be pointing to that set. What REALLOCATE_BITMAPSETS aims to protect against is negating the fact that almost all Bitmapsets in our tests will only ever have a single Bitmapword, so almost all modifications to sets won't do a repalloc(). The bms_copy_and_free() should help catch usages that fail to assign the value returned by one of the modify-one-of-the-inputs bms_*() functions back to the set being modified. Since bms_union() never modifies the input sets, REALLOCATE_BITMAPSETS does not/cannot apply. For the usages I was questioning, bms_union() was the first operation, so a new set was created before any possible in-place modifications to the set. In many of the cases, that was intentional and I was concerned that I had missed that intention, which I did in substitute_phv_relids_walker(), as Tom has pointed out. David
