Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: FOR PORTION OF does not recompute GENERATED STORED columns that
 depend on the range column
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CA+renyUTzwAMar173cbbxJypChp7s=txxgB+LYJQ5oRZ3a5hYQ@mail.gmail.com>
Date: Sat, 9 May 2026 14:38:03 +0800
Cc: Peter Eisentraut <peter@eisentraut.org>,
 jian he <jian.universality@gmail.com>,
 SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <D0DFAFD8-ADD8-45C5-ABEA-7CAC94E4B790@gmail.com>
References: 
 <CAHg+QDcd=t69gLf9yQexO07EJ2mx0Z70NFHo6h94X1EDA=hM0g@mail.gmail.com>
 <CACJufxGreOtA-S-qeHyS5iSSsj5zZX0W3Rf8FxbyL+SVXFjLYw@mail.gmail.com>
 <CAHg+QDeGLfz8YSCChjqrxaVSrz9AnMA0NrmsNogLqeGgCt7-wg@mail.gmail.com>
 <CA+renyWD+XXifwswE74vhjooqbiVKu4qVhLvpMcUQBzrjVjT7A@mail.gmail.com>
 <CACJufxHYntqy2fo9CFWDDrqKjcMK8DGRM3kse4YnXYnPYq2Hiw@mail.gmail.com>
 <CA+renyVp4rgj8x0ERXRkZp223eyBZ_XZr2RVCXvjzKBhTtS6Yw@mail.gmail.com>
 <CACJufxEkomKYmWgqXJmQr_qS+z=BZ3w801eh7Z7ekh-3oHXxHQ@mail.gmail.com>
 <CA+renyWk7kVsZJPZKzN95mYkO7S=hDUx=+fUPtbg9qFqeepCpg@mail.gmail.com>
 <CACJufxHai+HB1gkNqVEHe4oKyUmXfAagWBYAWXYKy8hyMV3RxA@mail.gmail.com>
 <CA+renyUBQdhnYxfPay+dxFs6BU1-fnEQskT0r-3dQ2v-ZnmZzg@mail.gmail.com>
 <CA+renyVZLYSghHb_85w0pUBG0KNGKTwciFTKBK5--rpHUM+VdA@mail.gmail.com>
 <e856c439-bf96-4244-8dc3-18b2a552e406@eisentraut.org>
 <CA+renyU6rNkiNGreMyQ7pU_F6-5RND5jchHbECH4NoRO7W0Q-Q@mail.gmail.com>
 <27BD5D23-19C9-4FD1-8935-9C788C3C9869@gmail.com>
 <66C1555B-CA54-4ED1-AB4F-0EE97D24A006@gmail.com>
 <91B35E0F-5DC1-4417-A1B9-FAF4A3DCD2BD@gmail.com>
 <CA+renyUBLAynaj0BKhajB6F=sLuitQkjT+_sOt5HBSRn82iQsw@mail.gmail.com>
 <74C1863C-2C2A-423A-BDE7-0228889F1D80@gmail.com>
 <CA+renyUTzwAMar173cbbxJypChp7s=txxgB+LYJQ5oRZ3a5hYQ@mail.gmail.com>
To: Paul A Jungwirth <pj@illuminatedcomputing.com>
Archived-At: 
 <https://www.postgresql.org/message-id/D0DFAFD8-ADD8-45C5-ABEA-7CAC94E4B790%40gmail.com>
Precedence: bulk


> On May 8, 2026, at 23:25, Paul A Jungwirth =
<pj@illuminatedcomputing.com> wrote:
>=20
> On Fri, May 8, 2026 at 12:10=E2=80=AFAM Chao Li =
<li.evan.chao@gmail.com> wrote:
>>> =
<v11-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v11-0002-Fi=
x-FOR-PORTION-OF-with-partitions-and-inheritan.patch>
>>=20
>> Thanks for updating the patch and making the separation. After =
reading v11, I still have a few comments for 0001.
>>=20
>> ```
>> +       if (relinfo->ri_forPortionOf)
>> +       {
>> +               AttrNumber  rangeAttno =3D =
relinfo->ri_forPortionOf->fp_rangeAttno;
>> +
>> +               if (!bms_is_member(rangeAttno - =
FirstLowInvalidHeapAttributeNumber,
>> +                                                  updatedCols))
>> +               {
>> +                       MemoryContext oldContext;
>> +
>> +                       oldContext =3D =
MemoryContextSwitchTo(estate->es_query_cxt);
>> +
>> +                       updatedCols =3D bms_copy(updatedCols);
>> +                       updatedCols =3D
>> +                               bms_add_member(updatedCols,
>> +                                                          rangeAttno =
- FirstLowInvalidHeapAttributeNumber);
>> +
>> +                       MemoryContextSwitchTo(oldContext);
>> +               }
>>        }
>> ```
>>=20
>> 1. I don=E2=80=99t think we should unconditionally do bms_copy, only =
if (updatedCols =3D=3D perminfo->updatedCols), we need to make the copy.
>=20
> You're saying we can skip the copy if execute_attr_map_cols already
> made a new bms above. That's true. Since we're going to just use the
> current memory context (see below), that seems safe.
>=20
>> 2. I doubt if we need to switch to estate->es_query_cxt. Because =
ExecGetUpdatedCols() is called by ExecGetAllUpdatedCols(), and its =
header comment says the function runs in per-tuple memory context:
>> ```
>> /*
>> * Return columns being updated, including generated columns
>> *
>> * The bitmap is allocated in per-tuple memory context. It's up to the =
caller to
>> * copy it into a different context with the appropriate lifespan, if =
needed.
>> */
>> Bitmapset *
>> ExecGetAllUpdatedCols(ResultRelInfo *relinfo, EState *estate)
>> ```
>>=20
>> So I think bms_copy and bms_add_member should be just done in the =
current memory context.
>=20
> Okay. I think using the current memory context is more correct anyway.
> There are other callers, and using the query memory context isn't
> necessarily what they want. Also the bms (potentially) allocated by
> execute_attr_map_cols is in the current memory context, so doing
> something different feels surprising. And it's safer not to change the
> behavior. Maybe there is a memory leak because of a long-lived
> context, but then it exists already. I added a comment to
> ExecGetUpdatedCols to call out that we use the current memory context.
>=20
>> 3. "rangeAttno - FirstLowInvalidHeapAttributeNumber=E2=80=9D appears =
twice, maybe add a local variable to avoid the duplication.
>=20
> Okay.
>=20
> v12 attached.
>=20
> Yours,
>=20
> --=20
> Paul              ~{:-)
> pj@illuminatedcomputing.com
> =
<v12-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v12-0002-Fi=
x-FOR-PORTION-OF-with-partitions-and-inheritan.patch>

Thanks for updating the patch. I have no more comment. V12 LGTM.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/