Re: Fixing a few minor misusages of bms_union()

On Sat, 4 Oct 2025 at 02:29, Greg Burd <greg@burd.me> wrote:
>
> On Oct 3 2025, at 5:36 am, David Rowley <dgrowleyml@gmail.com> 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