public inbox for [email protected]  

help / color / mirror / Atom feed

From: Haibo Yan <[email protected]>
To: jian he <[email protected]>
Cc: Andreas Karlsson <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: refactor ExecInitPartitionInfo
Date: Thu, 2 Apr 2026 15:19:47 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACJufxFX_pTuBnQKoDP56ChcRiYdN+54KZnf99vo7MqVWrXmYw@mail.gmail.com>
References: <CACJufxEN_mmgTtp-raJ9-VJHgyAGmb0SrO+01kn5by4mJ_XOfw@mail.gmail.com>
	<CACJufxGcUztOYcCj85o15YT68qPY3qpwUtdndqDgOs_Sps6Cow@mail.gmail.com>
	<[email protected]>
	<CACJufxFX_pTuBnQKoDP56ChcRiYdN+54KZnf99vo7MqVWrXmYw@mail.gmail.com>

> 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>

view thread (5+ messages)

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], [email protected]
  Subject: Re: refactor ExecInitPartitionInfo
  In-Reply-To: <[email protected]>

* 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