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: <66C1555B-CA54-4ED1-AB4F-0EE97D24A006@gmail.com>
Date: Thu, 7 May 2026 15:05:34 +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: <91B35E0F-5DC1-4417-A1B9-FAF4A3DCD2BD@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>
To: Paul A Jungwirth <pj@illuminatedcomputing.com>
Archived-At: 
 <https://www.postgresql.org/message-id/91B35E0F-5DC1-4417-A1B9-FAF4A3DCD2BD%40gmail.com>
Precedence: bulk


> On May 7, 2026, at 13:54, Chao Li <li.evan.chao@gmail.com> wrote:
>=20
>=20
>=20
>> On May 7, 2026, at 12:34, Chao Li <li.evan.chao@gmail.com> wrote:
>>=20
>>=20
>>=20
>>> On May 7, 2026, at 01:13, Paul A Jungwirth =
<pj@illuminatedcomputing.com> wrote:
>>>=20
>>> On Wed, May 6, 2026 at 4:39=E2=80=AFAM Peter Eisentraut =
<peter@eisentraut.org> wrote:
>>>>=20
>>>> On 05.05.26 23:50, Paul A Jungwirth wrote:
>>>>> On Wed, Apr 22, 2026 at 11:03=E2=80=AFAM Paul A Jungwirth
>>>>> <pj@illuminatedcomputing.com> wrote:
>>>>>>=20
>>>>>> Good catch! I removed that line in v7 (attached). I also included =
your
>>>>>> test change to compute the range len by hand. Also a rebase was
>>>>>> necessary after d3bba04154.
>>>>>=20
>>>>> This needed a rebase. v8 attached.
>>>>=20
>>>> This patch fails the injection_points/isolation test for me.  It =
looks
>>>> like it causes a server crash.  Check please.
>>>=20
>>> Sorry, I didn't have injection_points enabled, but now I see it too.
>>> The attached v9 fixes it.
>>>=20
>>> Yours,
>>>=20
>>> --=20
>>> Paul              ~{:-)
>>> pj@illuminatedcomputing.com
>>> <v9-0001-Fix-some-problems-with-UPDATE-FOR-PORTION-OF.patch>
>>=20
>> Hi Paul,
>>=20
>> I didn=E2=80=99t review this patch earlier because, from the subject, =
I thought it was only about recomputing generated stored columns. I just =
noticed that the patch also changes the inheritance-table path, and I =
posted another patch for the inheritance-table bug. Please see [1].
>>=20
>> I tried applying the new tests from my patch on top of this patch, =
and it looks like this patch still does not fix the multi-inheritance =
case.
>>=20
>> So I=E2=80=99d like to check with you how we should proceed. I think =
there are two options:
>>=20
>> 1. Keep this patch focused on the generated-column issue described in =
the subject, and use my patch to fix the inheritance-table bug.
>> 2. I can continue from this patch and extend it to fix the =
multi-inheritance case as well.
>>=20
>> Please let me know what you prefer.
>>=20
>> [1] =
https://www.postgresql.org/message-id/4245F94D-84F1-4E05-BF81-C458A6CF9901=
%40gmail.com
>>=20
>=20
> I just looked into v9 and made a fix in ExecInitForPortionOf() that =
resolves the bug with multi-inheritance tables. I also added a test case =
for that.
>=20
> The inheritance-table bug affects not only UPDATE, but also DELETE, so =
I added test cases for DELETE as well. Please see 0002 for my changes.
>=20
> To make each commit self-contained, would you mind moving the code for =
the inheritance-table fix to 0002? Then you can keep focusing on 0001, =
and I can continue working on 0002.
>=20
> PFA v10 - 0001 the same as v9. 0002 fixed a bug with multi-inheritance =
tables.
>=20
> (Note, in 0002, there is a comment format change around line 1496, =
that was done by pgindent.)
>=20
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>=20
>=20
>=20
>=20
> =
<v10-0001-Fix-some-problems-with-UPDATE-FOR-PORTION-OF.patch><v10-0002-Fix=
-FOR-PORTION-OF-on-inherited-children-with-di.patch>

A few comments for 0001:

1 - execUtils.c
```
+	updatedCols =3D perminfo->updatedCols;
+
 	/* Map the columns to child's attribute numbers if needed. */
 	if (relinfo->ri_RootResultRelInfo)
 	{
 		TupleConversionMap *map =3D =
ExecGetRootToChildMap(relinfo, estate);
=20
 		if (map)
-			return execute_attr_map_cols(map->attrMap, =
perminfo->updatedCols);
+			updatedCols =3D =
execute_attr_map_cols(map->attrMap, updatedCols);
+	}
+
+	/*
+	 * For UPDATE ... FOR PORTION OF, the range column is being =
modified
+	 * (narrowed via intersection), but it is not included in =
updatedCols
+	 * because the user does not need UPDATE permission on it. Now =
manualy
+	 * add it to updatedCols. Since ri_forPortionOf->fp_rangeAttno =
is already
+	 * mapped for the child partition, we have to add it after the =
mapping just
+	 * above. Also that makes it unsafe to mutate perminfo. XXX: =
Always add the
+	 * unmapped attno instead (before mapping), and mutate perminfo, =
to avoid
+	 * repeated allocations?
+	 */
+	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_add_member(updatedCols,
+							   rangeAttno - =
FirstLowInvalidHeapAttributeNumber);
```

The comment explicitly says that it is unsafe to mutate perminfo, but =
bms_add_member() does not always allocate a new bitmapset. So if =
updatedCols still points to perminfo->updatedCols, then bms_add_member() =
may mutate perminfo->updatedCols.

To verify that, I added Assert(updatedCols !=3D perminfo->updatedCols); =
right after the bms_add_member(), then ran "make check". A lot of tests =
failed, which seems to confirm that perminfo->updatedCols was being =
mutated.

So, I think we should make a copy the bitmapset before bms_add_member =
when needed, to make sure perminfo is not mutated, something like:
```
			if (updatedCols =3D=3D perminfo->updatedCols)
            			updatedCols =3D bms_copy(updatedCols);

			updatedCols =3D
				bms_add_member(updatedCols,
							   rangeAttno - =
FirstLowInvalidHeapAttributeNumber);
```

2 - execUtils.c
```
+  * because the user does not need UPDATE permission on it. Now manualy
```

Typo: manualy -> manually

3 - nodeModifyTable.c
```
+		/*
+		 * If we don't have a ForPortionOfState yet, we must be =
a partition
+		 * child being hit for the first time. Make a copy from =
the root, with
+		 * our own TupleTableSlot. We do this lazily so that we =
don't pay the
+		 * price of unused partitions.
+		 */
+		if ((((ModifyTable *) =
context.mtstate->ps.plan)->forPortionOf) &&
+			!resultRelInfo->ri_forPortionOf)
+		{
+			ExecInitForPortionOf(context.mtstate, estate, =
resultRelInfo);
+		}
```

I think this comment is stale. It could be a partition child or an =
inheritance child.

4 - nodeModifyTable.c
```
+	/* Each partition needs a slot matching its tuple descriptor */
+	leafState->fp_Existing =3D
+		table_slot_create(resultRelInfo->ri_RelationDesc,
+						  =
&mtstate->ps.state->es_tupleTable);
```

I think the comment should say "each child relation" rather than "each =
partition".

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