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

On Fri, May 8, 2026 at 11:25 PM 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:
> > ```

Switching to estate->es_query_cxt can actually save some cycles.

See ExecGetExtraUpdatedCols->ExecInitGenerated
    /*
     * Make sure these data structures are built in the per-query memory
     * context so they'll survive throughout the query.
     */
    oldContext = MemoryContextSwitchTo(estate->es_query_cxt);


In ExecGetUpdatedCols, we can change it to the following to save some
unnecessary bms_add_member cycle.
``````
    if (relinfo->ri_forPortionOf)
    {
        AttrNumber    rangeAttno = relinfo->ri_forPortionOf->fp_rangeAttno;

        if (!bms_is_member(rangeAttno - FirstLowInvalidHeapAttributeNumber,
                           updatedCols))
        {
            MemoryContext oldContext;

            if (updatedCols != perminfo->updatedCols)
                updatedCols = bms_add_member(updatedCols, rangeAttno -
FirstLowInvalidHeapAttributeNumber);
            else
            {
                oldContext = MemoryContextSwitchTo(estate->es_query_cxt);

                updatedCols = bms_add_member(updatedCols, rangeAttno -
FirstLowInvalidHeapAttributeNumber);

               MemoryContextSwitchTo(oldContext);
            }
        }
    }
``````