Re: Fixing a few minor misusages of bms_union()

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