Обсуждение: Fixing a few minor misusages of bms_union()

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

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

David Rowley <dgrowleyml@gmail.com> writes:
> Posting here to see if anyone knows a reason for not doing this that
> I've overlooked.

This change in substitute_phv_relids_walker is *not* safe according
to the routine's head comment:

 * NOTE: although this has the form of a walker, we cheat and modify the
 * nodes in-place.  This should be OK since the tree was copied by
 * pullup_replace_vars earlier.  Avoid scribbling on the original values of
 * the bitmapsets, though, because expression_tree_mutator doesn't copy those.

The change in generate_union_paths is obviously safe, though, since
that "relids" is entirely locally built.

I'm not convinced one way or the other about changing
markNullableIfNeeded.  I can't offhand think of a reason why
a Var would be sharing varnullingrels with some other node
at this point in the proceedings.  However, the comment
suggests that varnullingrels is probably NULL anyway, so that
there's nothing to be gained.

            regards, tom lane

On Oct 3 2025, at 10:04 am, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> David Rowley <dgrowleyml@gmail.com> writes:
>> Posting here to see if anyone knows a reason for not doing this that
>> I've overlooked.
> 
> This change in substitute_phv_relids_walker is *not* safe according
> to the routine's head comment:
> 
> * NOTE: although this has the form of a walker, we cheat and modify the
> * nodes in-place.  This should be OK since the tree was copied by
> * pullup_replace_vars earlier.  Avoid scribbling on the original
> values of
> * the bitmapsets, though, because expression_tree_mutator doesn't copy those.

I'll have to remember to scroll up a bit more when reviewing and always
read the header comments.  I missed that one entirely, apologies. When I
read the bitmapset_del() below the bitmapset_union() I incorrectly
assumed that it was okay to modify it in-place.  Maybe a short comment
above that line would be useful?

/*
 * The use of union here is purposeful as it will copy the bitmapset
 * thereby avoiding the potential for modifying the original set which
 * isn't copied by the expression_tree_mutator.
 */

Or maybe I should have just read the header comment. :)

> The change in generate_union_paths is obviously safe, though, since
> that "relids" is entirely locally built.
> 
> I'm not convinced one way or the other about changing
> markNullableIfNeeded.  I can't offhand think of a reason why
> a Var would be sharing varnullingrels with some other node
> at this point in the proceedings.  However, the comment
> suggests that varnullingrels is probably NULL anyway, so that
> there's nothing to be gained.
> 
>             regards, tom lane

best.

-greg

Greg Burd <greg@burd.me> writes:
> On Oct 3 2025, at 10:04 am, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This change in substitute_phv_relids_walker is *not* safe according
>> to the routine's head comment:

> I'll have to remember to scroll up a bit more when reviewing and always
> read the header comments.  I missed that one entirely, apologies. When I
> read the bitmapset_del() below the bitmapset_union() I incorrectly
> assumed that it was okay to modify it in-place.  Maybe a short comment
> above that line would be useful?

After we've done the union(), we know we have a singly-referenced
bitmapset, so it's safe for the second change to be in-place.

            regards, tom lane

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

On Sat, 4 Oct 2025 at 03:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This change in substitute_phv_relids_walker is *not* safe according
> to the routine's head comment:

Oh right. I'll leave that one.

> The change in generate_union_paths is obviously safe, though, since
> that "relids" is entirely locally built.
>
> I'm not convinced one way or the other about changing
> markNullableIfNeeded.  I can't offhand think of a reason why
> a Var would be sharing varnullingrels with some other node
> at this point in the proceedings.  However, the comment
> suggests that varnullingrels is probably NULL anyway, so that
> there's nothing to be gained.

I mainly wanted to adjust the generate_union_paths() one which was
just above where I had been hacking on the UNION short-circuiting
work. Coming here was mostly an effort in due diligence to find
locations making the same mistake. I think this might be enough due
diligence, so I'll just go and modify the generate_union_paths() and
leave the markNullableIfNeeded() alone.

Thanks for looking.

David