MIME-Version: 1.0
References: 
 <CAHg+QDcd=t69gLf9yQexO07EJ2mx0Z70NFHo6h94X1EDA=hM0g@mail.gmail.com>
 <CACJufxGreOtA-S-qeHyS5iSSsj5zZX0W3Rf8FxbyL+SVXFjLYw@mail.gmail.com>
 <CAHg+QDeGLfz8YSCChjqrxaVSrz9AnMA0NrmsNogLqeGgCt7-wg@mail.gmail.com>
 <CA+renyWD+XXifwswE74vhjooqbiVKu4qVhLvpMcUQBzrjVjT7A@mail.gmail.com>
 <CACJufxHYntqy2fo9CFWDDrqKjcMK8DGRM3kse4YnXYnPYq2Hiw@mail.gmail.com>
 <CA+renyVp4rgj8x0ERXRkZp223eyBZ_XZr2RVCXvjzKBhTtS6Yw@mail.gmail.com>
 <CACJufxEkomKYmWgqXJmQr_qS+z=BZ3w801eh7Z7ekh-3oHXxHQ@mail.gmail.com>
 <CA+renyWk7kVsZJPZKzN95mYkO7S=hDUx=+fUPtbg9qFqeepCpg@mail.gmail.com>
 <CACJufxHai+HB1gkNqVEHe4oKyUmXfAagWBYAWXYKy8hyMV3RxA@mail.gmail.com>
 <CA+renyUBQdhnYxfPay+dxFs6BU1-fnEQskT0r-3dQ2v-ZnmZzg@mail.gmail.com>
 <CA+renyVZLYSghHb_85w0pUBG0KNGKTwciFTKBK5--rpHUM+VdA@mail.gmail.com>
 <e856c439-bf96-4244-8dc3-18b2a552e406@eisentraut.org>
 <CA+renyU6rNkiNGreMyQ7pU_F6-5RND5jchHbECH4NoRO7W0Q-Q@mail.gmail.com>
 <27BD5D23-19C9-4FD1-8935-9C788C3C9869@gmail.com>
 <66C1555B-CA54-4ED1-AB4F-0EE97D24A006@gmail.com>
 <91B35E0F-5DC1-4417-A1B9-FAF4A3DCD2BD@gmail.com>
 <CA+renyUBLAynaj0BKhajB6F=sLuitQkjT+_sOt5HBSRn82iQsw@mail.gmail.com>
 <74C1863C-2C2A-423A-BDE7-0228889F1D80@gmail.com>
 <CA+renyUTzwAMar173cbbxJypChp7s=txxgB+LYJQ5oRZ3a5hYQ@mail.gmail.com>
In-Reply-To: 
 <CA+renyUTzwAMar173cbbxJypChp7s=txxgB+LYJQ5oRZ3a5hYQ@mail.gmail.com>
From: jian he <jian.universality@gmail.com>
Date: Mon, 11 May 2026 20:03:17 +0800
Message-ID: 
 <CACJufxFHe9iq50RfgyU3T1_CrB+NfdrjdBUp6NFNtb=Dy5Lf-g@mail.gmail.com>
Subject: Re: FOR PORTION OF does not recompute GENERATED STORED columns that
 depend on the range column
To: Paul A Jungwirth <pj@illuminatedcomputing.com>
Cc: Chao Li <li.evan.chao@gmail.com>, Peter Eisentraut <peter@eisentraut.org>,
	SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CACJufxFHe9iq50RfgyU3T1_CrB%2BNfdrjdBUp6NFNtb%3DDy5Lf-g%40mail.gmail.com>
Precedence: bulk

On Fri, May 8, 2026 at 11:25=E2=80=AFPM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Fri, May 8, 2026 at 12:10=E2=80=AFAM Chao Li <li.evan.chao@gmail.com> =
wrote:
> > > <v11-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v11-00=
02-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 =3D relinfo->ri_forPortionOf->fp=
_rangeAttno;
> > +
> > +               if (!bms_is_member(rangeAttno - FirstLowInvalidHeapAttr=
ibuteNumber,
> > +                                                  updatedCols))
> > +               {
> > +                       MemoryContext oldContext;
> > +
> > +                       oldContext =3D MemoryContextSwitchTo(estate->es=
_query_cxt);
> > +
> > +                       updatedCols =3D bms_copy(updatedCols);
> > +                       updatedCols =3D
> > +                               bms_add_member(updatedCols,
> > +                                                          rangeAttno -=
 FirstLowInvalidHeapAttributeNumber);
> > +
> > +                       MemoryContextSwitchTo(oldContext);
> > +               }
> >         }
> > ```
> >
> > 1. I don=E2=80=99t think we should unconditionally do bms_copy, only if=
 (updatedCols =3D=3D 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 ExecGe=
tUpdatedCols() 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 =3D 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 =3D relinfo->ri_forPortionOf->fp_rangeAttn=
o;

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

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

                updatedCols =3D bms_add_member(updatedCols, rangeAttno -
FirstLowInvalidHeapAttributeNumber);

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