Re: Wrong results with initplans moved by setrefs.c

On Mon, 9 Nov 2026 at 15:54, Richard Guo <guofenglinux(at)gmail(dot)com> 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