Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

> On May 8, 2026, at 23:25, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:
> 
> On Fri, May 8, 2026 at 12:10 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>> <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 the copy.
> 
> 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.
> 
>> 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.
> 
> 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.
> 
>> 3. "rangeAttno - FirstLowInvalidHeapAttributeNumber” appears twice, maybe add a local variable to avoid the duplication.
> 
> Okay.
> 
> v12 attached.
> 
> Yours,
> 
> -- 
> Paul              ~{:-)
> pj@illuminatedcomputing.com
> <v12-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v12-0002-Fix-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/