On Oct 3 2025, at 5:36 am, David Rowley <dgrowleyml@gmail.com> wrote:
> While working in prepunion.c, I noticed that generate_union_paths()
> has some code being called in a loop that's doing:
>
> relids = bms_union(relids, rel->relids);
>
> Since bms_union() creates a new set rather than reusing one of its
> input parameter sets, it makes this appear to be an inefficient way of
> accumulating the required set of relids. bms_add_members() seems
> better suited to this job.
Good idea, that seems like a win.
> From what I can tell, there are 2 other places where we do something
> similar: markNullableIfNeeded() and substitute_phv_relids_walker().
> These two are slightly different as they're not "accumulating"
> something in a loop like the above, but to me, they also look like
> they don't need to reallocate a completely new Bitmapset. I believe
> using bms_add_members() would only be an issue if there were multiple
> pointers pointing to the same set. In that case, modifying the set
> in-place might have an unintentional effect on the other pointers...
>
> 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?
> Posting here to see if anyone knows a reason for not doing this that
> I've overlooked.
>
> David
>
> (For the record, the other two cases I found that don't seem valid to
> change are in create_nestloop_plan() and finalize_plan()).
This seems like a reasonable change to me, +1.
best.
-greg
