Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1w8QOT-000WEf-0W for pgsql-hackers@arkaria.postgresql.org; Thu, 02 Apr 2026 22:20:05 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1w8QOR-008R2r-37 for pgsql-hackers@arkaria.postgresql.org; Thu, 02 Apr 2026 22:20:04 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1w8QOR-008R2j-24 for pgsql-hackers@lists.postgresql.org; Thu, 02 Apr 2026 22:20:04 +0000 Received: from mail-dy1-x132e.google.com ([2607:f8b0:4864:20::132e]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1w8QOP-00000000H0W-0jTU for pgsql-hackers@postgresql.org; Thu, 02 Apr 2026 22:20:03 +0000 Received: by mail-dy1-x132e.google.com with SMTP id 5a478bee46e88-2b6b0500e06so2119809eec.1 for ; Thu, 02 Apr 2026 15:20:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1775168399; x=1775773199; darn=postgresql.org; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=0MKuF98gwOCz4MlLu2XFlIWZ4f7al7Cf/T1zIW5u/hM=; b=SZB6YntzGp7scRg/007247Rf0vXNR9E/auOSUA590SyyZ52OWQR9GnU7UhWOB5Fr7V MdHeSiEpUjdD1UDhA5Puy8LM0dsGDHm7srU7ukO/YIdYlwVdGivbR6cuzYPfKzS3C6gS bjTHdYJ4+hKvnqkoC3eqYqOh3IdMd4UdZzz9/W8mGK5KqGC5vMvjDWP9f5DVajPGKk5C zoCIYagrmQ2y7aESqD+sv4johonDyVDDThXKIlhf6jTJqpDVmlA4bA8mlcGbPMp6iNIF 2peAdJCSPrGIQm2JL58PmMAcbBLX194oZYAaGj5J3LoljKhiGmzhtPyHjVz9EH9VRmu4 Cxcw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1775168399; x=1775773199; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=0MKuF98gwOCz4MlLu2XFlIWZ4f7al7Cf/T1zIW5u/hM=; b=hV1VOdMPiUBM2xXyYoFX7eKUX1e7dycc2Oe7mwfFoPeIBMpvOwsSDrWflx3m3ZZPRf vPtYfQ5l9WEYmWzld5nR56qEl35BjYzs9FfkwkmZQk1caQmIwARTvr3p/HCtT0MSDQLb hlSRx+6igPsp9WIr8dJofqy/Ku1SNJxqX0+LrUljsHpfJxe0YMBYXgTVwBoGNriyzVvB XmjvnZxEUREyKO1NCWZBMaZ6GUkDvnw79UJMKrlNsb/eFLnM0+nt1sLP/lWJCP0mMM1F x/+KXya6ewceyInnFSoK7r8RYdwXe/auVlpFb/78aPgdKwYZeBhF3wpJN5p7DVjVS3Jz aGNQ== X-Forwarded-Encrypted: i=1; AJvYcCUQaT+Yo8natRCLtUq+b4QLU/ADt/d7afHLLg/bhaWzvblwaTCmslRzIqpRowgpfStKXIHg3jDmQ+VwNu0Y@postgresql.org X-Gm-Message-State: AOJu0Yx9DEksl7H0rspyZC3HUZ5qBX2yaf/vg69zTxyWkHL8shhnKWp0 Dbk5k4BjuIsSr7gp0Te1pu+LPfLD6N1qFwOLu98bXPpeVlce1bDcwrW/ X-Gm-Gg: AeBDievNzzamZrX4nZOZBn7+CikfrkjN+MOVNr1ET0WTgdwzI8xj1EUb788xGIRAZgL CrHwFXUMxG0Bnzq2mB/wrp6kIigoOXljELMVDurCCDTx6ZNtoN0XQS88AEFynTZoj7y4BbqwfI9 Z11G3YwE6vk1zlTe48nYT82c8OMZFkAM5pMrOjN4S4BDX4lTXgcCW8Y4s6AIpM8P158JwT2vHez TtORbeDj9gq1Efu8i3YGmrUmqcGDhq7yYXC5iKzHBPUlCk+YoAhJRmuHR6uk4ff09vy1FlYG1aH 8XXT6KWJ4Ea7yHfFT5UwhWj5mPDtq6pEoQTn+lA7hSszHxGX1UUGbqjLen36vPk6fnPixIYzGYX 8ZjdnWU9ih1fDUdrORJ/1xGz4M02t0F0yAwwZ5c/PQN/nsypJXHLQD56ExcdIngUBx72qIkI1xa GfvAELl3G5hWQnclEmeA3N9j39g9FBP4k6RmFpWxdLmyUSHtdmMNJDUG8NXx5ccQ== X-Received: by 2002:a05:7300:571e:b0:2c5:3b87:2fe9 with SMTP id 5a478bee46e88-2cbfa4c7f62mr449542eec.9.1775168398714; Thu, 02 Apr 2026 15:19:58 -0700 (PDT) Received: from smtpclient.apple ([2620:149:13d0::ccf]) by smtp.gmail.com with ESMTPSA id 5a478bee46e88-2cb92ea0ef1sm1463275eec.21.2026.04.02.15.19.58 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 02 Apr 2026 15:19:58 -0700 (PDT) From: Haibo Yan X-Google-Original-From: Haibo Yan Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_F910F94A-4BA7-4A93-B59D-1A3850B8FEC3" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.500.181\)) Subject: Re: refactor ExecInitPartitionInfo Date: Thu, 2 Apr 2026 15:19:47 -0700 In-Reply-To: Cc: Andreas Karlsson , PostgreSQL-development To: jian he References: X-Mailer: Apple Mail (2.3864.500.181) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --Apple-Mail=_F910F94A-4BA7-4A93-B59D-1A3850B8FEC3 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Apr 1, 2026, at 6:12=E2=80=AFAM, jian he = wrote: >=20 > On Mon, Feb 23, 2026 at 10:28=E2=80=AFAM Andreas Karlsson = wrote: >>=20 >>> if (node !=3D NULL) >>> part_attmap =3D = build_attrmap_by_name(RelationGetDescr(partrel), >>> = RelationGetDescr(firstResultRel), >>> false); >>>=20 >>> We have now consolidated five uses of build_attrmap_by_name into = one. >>=20 >> 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? >>=20 >=20 > Ok. I switched it back to >=20 > + if (node && > + (node->withCheckOptionLists !=3D NIL || > + node->returningLists !=3D NIL || > + node->onConflictAction =3D=3D ONCONFLICT_UPDATE || > + node->onConflictWhere || > + node->operation =3D=3D CMD_MERGE)) > + { > + /* later map_variable_attnos need use attribute map, build it = now */ > + part_attmap =3D = build_attrmap_by_name(RelationGetDescr(partrel), > + = RelationGetDescr(firstResultRel), > + false); > + } > + >=20 Hi Jian, Thanks for working on this. I=E2=80=99m 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 = =3D=3D 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=E2=80=99m 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/ > --Apple-Mail=_F910F94A-4BA7-4A93-B59D-1A3850B8FEC3 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
On = Apr 1, 2026, at 6:12=E2=80=AFAM, jian he = <jian.universality@gmail.com> wrote:

On Mon, Feb 23, 2026 at = 10:28=E2=80=AFAM Andreas Karlsson <andreas@proxel.se> = wrote:

=     if (node !=3D NULL)
=         part_attmap =3D = build_attrmap_by_name(RelationGetDescr(partrel),
=             &n= bsp;           &nbs= p;            =        RelationGetDescr(firstResultRel)= ,
=             &n= bsp;           &nbs= p;            =        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 = !=3D NIL ||
+ =         node->returningLists = !=3D NIL ||
+ =         node->onConflictAction = =3D=3D ONCONFLICT_UPDATE ||
+ =         node->onConflictWhere = ||
+ =         node->operation =3D=3D = CMD_MERGE))
+    {
+ =        /* later map_variable_attnos = need use attribute map, build it now */
+ =        part_attmap =3D = build_attrmap_by_name(RelationGetDescr(partrel),
+ =             &n= bsp;           &nbs= p;            =       RelationGetDescr(firstResultRel),
+= =             &n= bsp;           &nbs= p;            =       false);
+ =    }
+

Hi Jian,

Thanks for working on = this.

I=E2=80=99m 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:

  1. it stays lazy, so we only build part_attmap in paths that actually need it, = and

  2. 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 =3D=3D = 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=E2=80=99m 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>= ;

= --Apple-Mail=_F910F94A-4BA7-4A93-B59D-1A3850B8FEC3--