Re: refactor ExecInitPartitionInfo

> On Apr 1, 2026, at 6:12 AM, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> On Mon, Feb 23, 2026 at 10:28 AM Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>>
>>> if (node != NULL)
>>> part_attmap = build_attrmap_by_name(RelationGetDescr(partrel),
>>> RelationGetDescr(firstResultRel),
>>> false);
>>>
>>> We have now consolidated five uses of build_attrmap_by_name into one.
>>
>> Hm, why would that be ok? As far as I can tell the current code tries
>> hard to not build the attmap unless it is actually needed while you
>> propose to build it almost unconditionally. While the code is less
>> complicated with your patch it instead has to do more work in some
>> cases, right?
>>
>
> Ok. I switched it back to
>
> + if (node &&
> + (node->withCheckOptionLists != NIL ||
> + node->returningLists != NIL ||
> + node->onConflictAction == ONCONFLICT_UPDATE ||
> + node->onConflictWhere ||
> + node->operation == CMD_MERGE))
> + {
> + /* later map_variable_attnos need use attribute map, build it now */
> + part_attmap = build_attrmap_by_name(RelationGetDescr(partrel),
> + RelationGetDescr(firstResultRel),
> + false);
> + }
> +
>
Hi Jian,

Thanks for working on this.

I’m not convinced this refactoring buys us much in its current form.

The original code is a bit repetitive, but it has two properties that seem valuable here:

it stays lazy, so we only build part_attmap in paths that actually need it, and

the initialization stays local to the corresponding use sites, which makes the control flow easier to follow.

With the consolidated version, we reduce a few repeated if (part_attmap == NULL) checks, but I do not think that gives us a meaningful improvement in either performance or readability. In fact, the centralized predicate arguably makes the code a bit harder to reason about, because the knowledge of which paths require an attribute map is no longer kept next to the actual remapping logic.

That also seems a little more fragile for future maintenance. If a new path is added later that needs part_attmap, it is easier to miss updating the centralized condition than it is to keep the existing lazy initialization near the place where it is used.

So from my perspective, this is not really a performance optimization, and I’m also not sure it is a net cleanup. The old shape is somewhat repetitive, but it is straightforward and local. Unless this is needed as part of a larger follow-up series that will add several more such call sites, I would be inclined to keep the current pattern.

Regards,

Haibo

> --
> jian
> https://www.enterprisedb.com/
> <v3-0001-refactor-ExecInitPartitionInfo.patch>