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
makethe 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
