Subject: Re: refactor ExecInitPartitionInfo

From: jian he <[email-masked]>
Date: Sat, 21 Feb 2026 23:07:52 +0800

On Fri, Nov 28, 2025 at 7:00 PM jian he <[email-masked]> 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/
From 3b7eb284241675a1026eeebc9a6f682a88f95430 Mon Sep 17 00:00:00 2001
From: jian he <[email-masked]>
Date: Sat, 21 Feb 2026 22:50:55 +0800
Subject: [PATCH v2 1/1] refactor ExecInitPartitionInfo

Discussion: https://postgr.es/m/[email-masked]
commitfest: https://commitfest.postgresql.org/patch/6280
---
 src/backend/executor/execPartition.c | 35 +++++-----------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index bab294f5e91..d62d6a71f03 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -607,6 +607,11 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 						(node != NULL &&
 						 node->onConflictAction != ONCONFLICT_NONE));
 
+	if (node != NULL)
+		part_attmap = build_attrmap_by_name(RelationGetDescr(partrel),
+											RelationGetDescr(firstResultRel),
+											false);
+
 	/*
 	 * Build WITH CHECK OPTION constraints for the partition.  Note that we
 	 * didn't build the withCheckOptionList for partitions within the planner,
@@ -649,10 +654,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 		/*
 		 * Convert Vars in it to contain this partition's attribute numbers.
 		 */
-		part_attmap =
-			build_attrmap_by_name(RelationGetDescr(partrel),
-								  RelationGetDescr(firstResultRel),
-								  false);
 		wcoList = (List *)
 			map_variable_attnos((Node *) wcoList,
 								firstVarno, 0,
@@ -706,14 +707,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 		 */
 		returningList = linitial(node->returningLists);
 
-		/*
-		 * Convert Vars in it to contain this partition's attribute numbers.
-		 */
-		if (part_attmap == NULL)
-			part_attmap =
-				build_attrmap_by_name(RelationGetDescr(partrel),
-									  RelationGetDescr(firstResultRel),
-									  false);
 		returningList = (List *)
 			map_variable_attnos((Node *) returningList,
 								firstVarno, 0,
@@ -951,11 +944,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 					List	   *onconflcols;
 
 					onconflset = copyObject(node->onConflictSet);
-					if (part_attmap == NULL)
-						part_attmap =
-							build_attrmap_by_name(RelationGetDescr(partrel),
-												  RelationGetDescr(firstResultRel),
-												  false);
+
 					onconflset = (List *)
 						map_variable_attnos((Node *) onconflset,
 											INNER_VAR, 0,
@@ -1007,12 +996,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 				{
 					List	   *clause;
 
-					if (part_attmap == NULL)
-						part_attmap =
-							build_attrmap_by_name(RelationGetDescr(partrel),
-												  RelationGetDescr(firstResultRel),
-												  false);
-
 					clause = copyObject((List *) node->onConflictWhere);
 					clause = (List *)
 						map_variable_attnos((Node *) clause,
@@ -1064,12 +1047,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 		ExprContext *econtext = mtstate->ps.ps_ExprContext;
 		Node	   *joinCondition;
 
-		if (part_attmap == NULL)
-			part_attmap =
-				build_attrmap_by_name(RelationGetDescr(partrel),
-									  RelationGetDescr(firstResultRel),
-									  false);
-
 		if (unlikely(!leaf_part_rri->ri_projectNewInfoValid))
 			ExecInitMergeTupleSlots(mtstate, leaf_part_rri);
 
-- 
2.34.1

---

From: Andreas Karlsson <[email-masked]>
Date: Mon, 23 Feb 2026 03:28:57 +0100

> 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

---

From: jian he <[email-masked]>
Date: Wed, 1 Apr 2026 21:12:54 +0800

> complicated with your patch it instead has to do more work in some

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);
+    }
+

--
jian
https://www.enterprisedb.com/
From 80952eb3df518f8a01ea255710f9d960a453e08a Mon Sep 17 00:00:00 2001
From: jian he <[email-masked]>
Date: Wed, 1 Apr 2026 21:07:57 +0800
Subject: [PATCH v3 1/1] refactor ExecInitPartitionInfo

Optimize AttrMap creation in ExecInitPartitionInfo. Since map_variable_attnos is
called multiple times and requires an AttrMap, we can avoid redundant work by
building the map once with build_attrmap_by_name and reusing it.

Discussion: https://postgr.es/m/[email-masked]
commitfest: https://commitfest.postgresql.org/patch/6280
---
 src/backend/executor/execPartition.c | 40 ++++++++++------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index d96d4f9947b..70f299da019 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -608,6 +608,19 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 						(node != NULL &&
 						 node->onConflictAction != ONCONFLICT_NONE));
 
+	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);
+	}
+
 	/*
 	 * Build WITH CHECK OPTION constraints for the partition.  Note that we
 	 * didn't build the withCheckOptionList for partitions within the planner,
@@ -650,10 +663,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 		/*
 		 * Convert Vars in it to contain this partition's attribute numbers.
 		 */
-		part_attmap =
-			build_attrmap_by_name(RelationGetDescr(partrel),
-								  RelationGetDescr(firstResultRel),
-								  false);
 		wcoList = (List *)
 			map_variable_attnos((Node *) wcoList,
 								firstVarno, 0,
@@ -710,11 +719,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 		/*
 		 * Convert Vars in it to contain this partition's attribute numbers.
 		 */
-		if (part_attmap == NULL)
-			part_attmap =
-				build_attrmap_by_name(RelationGetDescr(partrel),
-									  RelationGetDescr(firstResultRel),
-									  false);
 		returningList = (List *)
 			map_variable_attnos((Node *) returningList,
 								firstVarno, 0,
@@ -952,11 +956,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 					List	   *onconflcols;
 
@@ -1008,12 +1008,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 				{
 					List	   *clause;
 
@@ -1065,12 +1059,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,

-- 
2.34.1

---

From: Haibo Yan <[email-masked]>
Date: Thu, 2 Apr 2026 15:19:47 -0700

> On Apr 1, 2026, at 6:12 AM, jian he <[email-masked]> wrote:
> 
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

> <v3-0001-refactor-ExecInitPartitionInfo.patch>