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

> On May 8, 2026, at 07:47, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:
>
> On Thu, May 7, 2026 at 12:06 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>>
<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
>> The comment explicitly says that it is unsafe to mutate perminfo, but bms_add_member() does not always allocate a
newbitmapset. 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
"makecheck". 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);
>> ```
>
> Ah, thanks for catching this! Fixed.
>
>> 2 - execUtils.c
>> ```
>> +  * because the user does not need UPDATE permission on it. Now manualy
>> ```
>>
>> Typo: manualy -> manually
>
> Fixed.
>
>> 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.
>
> Okay.
>
>> 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".
>
> Okay.
>
> In these v11 patches I've tried to separate (1) the fix for GENERATED
> STORED columns and UPDATE OF triggers (2) fixing inheritance and (and
> partitions too, for the bugs in #1). I understand why jian he combined
> these into one patch: there is some overlap. If you don't like my
> separation, let me know.
>
> Yours,
>
> --
> Paul              ~{:-)
> pj@illuminatedcomputing.com
>
<v11-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v11-0002-Fix-FOR-PORTION-OF-with-partitions-and-inheritan.patch>

Thanks for updating the patch and making the separation. After reading v11, I still have a few comments for 0001.

```
+    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_copy(updatedCols);
+            updatedCols =
+                bms_add_member(updatedCols,
+                               rangeAttno - FirstLowInvalidHeapAttributeNumber);
+
+            MemoryContextSwitchTo(oldContext);
+        }
     }
```

1. I don’t think we should unconditionally do bms_copy, only if (updatedCols == perminfo->updatedCols), we need to make
thecopy. 

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

So I think bms_copy and bms_add_member should be just done in the current memory context.

3. "rangeAttno - FirstLowInvalidHeapAttributeNumber” appears twice, maybe add a local variable to avoid the
duplication.

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