On Mon, 9 Nov 2026 at 15:54, Richard Guo <[email protected]> wrote: > I think the fix is simply to merge the removed node's extParam and > allParam into the child's, something like below in > clean_up_removed_plan_level(): > > child->initPlan = list_concat(parent->initPlan, > child->initPlan); > + > + child->extParam = bms_union(child->extParam, parent->extParam); > + child->allParam = bms_union(child->allParam, parent->allParam);
Looking at finalize_plan(), I'd say that's correct. But shouldn't it be bms_add_member() rather than bms_union()? While mostly harmless, I don't think creating a new set is the correct thing to do here. I think doing bms_union() where the variable being assigned is the same as the bms_union's first parameter is only valid in cases where you don't want the first parameter to be modified, e.g in cases like createplan.c:4325 where req_outer points to some field in a struct. I think all the cases where we're assigning bms_union's return to a struct field and that field is the same as the bms_union's first input are probably all candidates for using bms_add_member() instead. REALLOCATE_BITMAPSETS was designed to help identify cases where Bitmapset pointers are being shared. I'm personally not so keen on adjusting the other existing ones due to the poor risk to reward ratio, but since you're adding these lines, I'd say bms_add_member is a better option. David
