Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column - Mailing list pgsql-hackers

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

         if (map)
-            return execute_attr_map_cols(map->attrMap, perminfo->updatedCols);
+            updatedCols = 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 = relinfo->ri_forPortionOf->fp_rangeAttno;
+
+        if (!bms_is_member(rangeAttno - FirstLowInvalidHeapAttributeNumber,
+                           updatedCols))
+        {
+            MemoryContext oldContext;
+
+            oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
+
+            updatedCols =
+                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 != 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 == perminfo->updatedCols)
                        updatedCols = bms_copy(updatedCols);

            updatedCols =
                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 =
+        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/