Re: Fixing a few minor misusages of bms_union()

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

> David Rowley <dgrowleyml(at)gmail(dot)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