On Oct 3 2025, at 10:04 am, Tom Lane <[email protected]> wrote:
> David Rowley <[email protected]> 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
