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
