public inbox for [email protected]
help / color / mirror / Atom feedrefactor ExecInitPartitionInfo
3+ messages / 2 participants
[nested] [flat]
* refactor ExecInitPartitionInfo
@ 2025-11-28 11:00 jian he <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: jian he @ 2025-11-28 11:00 UTC (permalink / raw)
To: pgsql-hackers
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/
Attachments:
[text/x-patch] v1-0001-refactor-ExecInitPartitionInfo.patch (3.0K, 2-v1-0001-refactor-ExecInitPartitionInfo.patch)
download | inline diff:
From 32c9323b68fd6abb45dc0edec0a65a7dd076cef7 Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
Date: Fri, 28 Nov 2025 18:50:35 +0800
Subject: [PATCH v1 1/1] refactor ExecInitPartitionInfo
Discussion: https://postgr.es/m/
---
src/backend/executor/execPartition.c | 36 ++++++++++------------------
1 file changed, 13 insertions(+), 23 deletions(-)
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 0dcce181f09..f855d2c235c 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -546,6 +546,18 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
(node != NULL &&
node->onConflictAction != ONCONFLICT_NONE));
+ 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);
+ }
+
/*
* Build WITH CHECK OPTION constraints for the partition. Note that we
* didn't build the withCheckOptionList for partitions within the planner,
@@ -588,10 +600,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,
@@ -645,14 +653,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,
@@ -791,11 +791,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
* target relation (firstVarno).
*/
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,
@@ -891,12 +887,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
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: refactor ExecInitPartitionInfo
@ 2026-04-01 13:12 jian he <[email protected]>
parent: jian he <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: jian he @ 2026-04-01 13:12 UTC (permalink / raw)
To: Andreas Karlsson <[email protected]>; +Cc: pgsql-hackers
On Mon, Feb 23, 2026 at 10:28 AM Andreas Karlsson <[email protected]> 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);
+ }
+
--
jian
https://www.enterprisedb.com/
Attachments:
[text/x-patch] v3-0001-refactor-ExecInitPartitionInfo.patch (3.8K, 2-v3-0001-refactor-ExecInitPartitionInfo.patch)
download | inline diff:
From 80952eb3df518f8a01ea255710f9d960a453e08a Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
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/CACJufxEN_mmgTtp-raJ9-VJHgyAGmb0SrO+01kn5by4mJ_XOfw@mail.gmail.com
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;
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,
@@ -1008,12 +1008,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,
@@ -1065,12 +1059,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
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: refactor ExecInitPartitionInfo
@ 2026-04-02 22:19 Haibo Yan <[email protected]>
parent: jian he <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Haibo Yan @ 2026-04-02 22:19 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Andreas Karlsson <[email protected]>; pgsql-hackers
> On Apr 1, 2026, at 6:12 AM, jian he <[email protected]> wrote:
>
> On Mon, Feb 23, 2026 at 10:28 AM Andreas Karlsson <[email protected]> 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>
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-04-02 22:19 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-11-28 11:00 refactor ExecInitPartitionInfo jian he <[email protected]>
2026-04-01 13:12 ` jian he <[email protected]>
2026-04-02 22:19 ` Haibo Yan <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox