Thread
-
refactor ExecInitPartitionInfo
jian he <jian.universality@gmail.com> — 2025-11-28T11:00:08Z
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/ -
Re: refactor ExecInitPartitionInfo
jian he <jian.universality@gmail.com> — 2026-02-21T15:07:52Z
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/ -
Re: refactor ExecInitPartitionInfo
Andreas Karlsson <andreas@proxel.se> — 2026-02-23T02:28:57Z
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
-
Re: refactor ExecInitPartitionInfo
jian he <jian.universality@gmail.com> — 2026-04-01T13:12:54Z
On Mon, Feb 23, 2026 at 10:28 AM Andreas Karlsson <andreas@proxel.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); + } + -- jian https://www.enterprisedb.com/ -
Re: refactor ExecInitPartitionInfo
Haibo Yan <tristan.yim@gmail.com> — 2026-04-02T22:19:47Z
> On Apr 1, 2026, at 6:12 AM, jian he <jian.universality@gmail.com> wrote: > > On Mon, Feb 23, 2026 at 10:28 AM Andreas Karlsson <andreas@proxel.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> -
Re: refactor ExecInitPartitionInfo
Heikki Linnakangas <hlinnaka@iki.fi> — 2026-07-06T11:03:11Z
On 03/04/2026 01:19, Haibo Yan wrote: >> On Apr 1, 2026, at 6:12 AM, jian he <jian.universality@gmail.com> wrote: >> 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. +1, for all the reasons you said. I have marked this as rejected in the commitfest app. Thanks! - Heikki