Обсуждение: refactor ExecInitPartitionInfo

hi.

I noticed the following code pattern repeated several times in
ExecInitPartitionInfo.
```
if (part_attmap == NULL)
            build_attrmap_by_name(RelationGetDescr(partrel),
                                  RelationGetDescr(firstResultRel),
                                  false);
```

we can consolidated into one, like:

+    if (node != NULL &&
+        (list_length(node->withCheckOptionLists) > 0 ||
+         list_length(node->returningLists) > 0 ||
+         node->onConflictAction != ONCONFLICT_NONE ||
+         node->operation == CMD_MERGE))
+    {
+        part_attmap =
+            build_attrmap_by_name(RelationGetDescr(partrel),
+                                  RelationGetDescr(firstResultRel),
+                                  false);
+    }
+

it matters, because nearby patch ON CONFLICT DO SELECT is= going to add another
one.

what do you think?

--
jian
https://www.enterprisedb.com/

On Fri, Nov 28, 2025 at 7:00 PM jian he <jian.universality@gmail.com> wrote:
>
> hi.
>
> I noticed the following code pattern repeated several times in
> ExecInitPartitionInfo.
> ```
> if (part_attmap == NULL)
>             build_attrmap_by_name(RelationGetDescr(partrel),
>                                   RelationGetDescr(firstResultRel),
>                                   false);
> ```
>
> we can consolidated into one, like:
>
> +    if (node != NULL &&
> +        (list_length(node->withCheckOptionLists) > 0 ||
> +         list_length(node->returningLists) > 0 ||
> +         node->onConflictAction != ONCONFLICT_NONE ||
> +         node->operation == CMD_MERGE))
> +    {
> +        part_attmap =
> +            build_attrmap_by_name(RelationGetDescr(partrel),
> +                                  RelationGetDescr(firstResultRel),
> +                                  false);
> +    }
> +
>


> +    if (node != NULL &&
> +        (list_length(node->withCheckOptionLists) > 0 ||
> +         list_length(node->returningLists) > 0 ||
> +         node->onConflictAction != ONCONFLICT_NONE ||
> +         node->operation == CMD_MERGE))
V1 is way too complicated.
IMHO, we can just do

    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.



--
jian
https://www.enterprisedb.com/

On 2/21/26 4:07 PM, jian he wrote:
> V1 is way too complicated.
> IMHO, we can just do
> 
>      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?

Andreas