public inbox for [email protected]
help / color / mirror / Atom feedFrom: jian he <[email protected]>
To: Andreas Karlsson <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: refactor ExecInitPartitionInfo
Date: Wed, 1 Apr 2026 21:12:54 +0800
Message-ID: <CACJufxFX_pTuBnQKoDP56ChcRiYdN+54KZnf99vo7MqVWrXmYw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CACJufxEN_mmgTtp-raJ9-VJHgyAGmb0SrO+01kn5by4mJ_XOfw@mail.gmail.com>
<CACJufxGcUztOYcCj85o15YT68qPY3qpwUtdndqDgOs_Sps6Cow@mail.gmail.com>
<[email protected]>
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
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: refactor ExecInitPartitionInfo
In-Reply-To: <CACJufxFX_pTuBnQKoDP56ChcRiYdN+54KZnf99vo7MqVWrXmYw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox