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.
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.
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()).
